date:20141124

On Mon, Nov 24, 2014 at 02:50:04AM +0300, Роман Донченко wrote:

 The RFC says that they are to be concatenated after decoding (i.e. the
 intervening whitespace is ignored).
 
 I change the sender's name to an all-Cyrillic string in the tests so that
 its encoded form goes over the 76 characters in a line limit, forcing
 format-patch to split it into multiple encoded words.
 
 Since I have to modify the regular expression for an encoded word anyway,
 I take the opportunity to bring it closer to the spec, most notably
 disallowing embedded spaces and making it case-insensitive (thus allowing
 the encoding to be specified as both q and Q).

The overall goal makes sense to me. Thanks for working on this. I have a
few questions/comments, though.

  sub unquote_rfc2047 {
   local ($_) = @_;
 +
 + my $et = qr/[!-@-~]+/; # encoded-text from RFC 2047
 + my $sep = qr/[ \t]+/;
 + my $encoded_word = qr/=\?($et)\?q\?($et)\?=/i;

The first $et in $encoded_word is actually the charset, which is defined
by RFC 2047 as:

 charset = token; see section 3

 token = 1*Any CHAR except SPACE, CTLs, and especials

 especials = ( / ) /  /  / @ / , / ; / : / 
/ / / [ / ] / ? / . / =

Your regex is a little more liberal. I doubt that it is a big deal in
practice (actually, in practice, I suspect [a-zA-Z0-9-] would be fine).
But if we are tightening things up in general, it may make sense to do
so here (and I notice that is_rfc2047_quoted does a more thorough $token
definition, and it probably makes sense for the two functions to be
consistent).

For your definition of encoded-text, RFC 2047 says:

 encoded-text = 1*Any printable ASCII character other than ?
  or SPACE

It looks like you pulled the definition of $et from is_rfc2047_quoted,
but I am not clear on where that original came from (it is from a3a8262,
but that commit message does not explain the regex).

Also, I note that we handle 'q'-style encodings here, but not 'b'. I
wonder if it is worth adding that in while we are in the area (it is not
a big deal if you always send-email git-generated patches, as we never
generate it).

 + s{$encoded_word(?:$sep$encoded_word)+}{

If I am reading this right, it requires at least two $encoded_words.
Should this + be a *?

 + my @words = split $sep, $;
 + foreach (@words) {
 + m/$encoded_word/;
 + $encoding = $1;
 + $_ = $2;
 + s/_/ /g;
 + s/=([0-9A-F]{2})/chr(hex($1))/eg;

In the spirit of your earlier change, should this final regex be
case-insensitive? RFC 2047 says only Upper case should be used for
hexadecimal digits A through F. but that does not seem like a MUST
to me.

-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] send-email: handle adjacent RFC 2047-encoded words properly

On Sun, Nov 23, 2014 at 11:27:51PM -0800, Junio C Hamano wrote:

 Was the change to the test to use Cyrillic really necessary, or did it
 suffice if you simply extended the existsing Funny Name spelled with
 strange accents, but you substituted the whole string anyway?
 
 Until I found out what the new string says by running web-based
 translation on it, I felt somewhat uneasy. As I do not read
 Cyrillic/Russian, we may have been adding some profanity without
 knowing. It turns out that the string just says Cyrillic Name, so I am
 not against using the new string, but it simply looked odd to replace the
 string whole-sale when you merely need a longer string. It made it look
 as if a bug was specific to Cyrillic when it wasn't.

I do not mind hidden Cyrillic profanity[1], but I found the new text
much harder to verify, because the shapes are very unfamiliar to my
eyes. I'd prefer if we can stick to accented Roman letters.  I realize
this is me being totally Anglo-centric. But for Cyrillic readers,
consider how much more difficult it would be to manually verify the test
if it were written in an unfamiliar script (e.g., Hangul).  The
surrounding code is already written in Roman characters (and English),
so it probably makes sense as a common denominator.

-Peff

[1] As long as it is only crude and not mean. :)
--
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: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?

On Mon, Nov 24, 2014 at 11:15:34AM +0100, Michael J Gruber wrote:

  I wonder if we can have an option to sign all blob content of the tree
  associated to a commit, and the content of parent commit(s). It's more
  expensive than signing just commit/tag content. But it's also safer
  without completely ditching SHA-1.
  
 
 This amounts to hashing the blob content with whatever hash you told
 your gpg to use (hopefully not sha1 ;) ) and signing that.

Right. You could also create a graph of SHA-256 (or whatever) object
hashes and sign that. I.e., create a parallel to git's trees using
SHA-256 and include a single:

  object-256 

line in the tag header. That still involves re-hashing all of the data,
but it would at least be possible to cache (i.e., a mapping of SHA-1 to
SHA-256 hashes). Of course one way to keep that caching layer up to date
would be to just calculate the SHA-256 along with the SHA-1 whenever we
create an object. And then you can sprinkle SHA-256 links in other
places, too, like commit objects.

And now you are halfway down the road to a combined SHA-1/SHA-256 git.
:)

The tricky thing is fitting the extra hash into the tree objects. And of
course the rules for actually generating and/or sending extra objects.

-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 RFC] CYGWIN: avoid implicit declaration warning

 
 It depends what we mean with old:
 cygwin 1.5 is old, and I lost my test installation this summer:
 One netbook was converted from XP to Linux, the other machine needs to be
 re-installed and CYGWIN 1.5 is no longer available for download.
 
 I can confirm that Ramsays patch works with CYGWIN 1.7 32 Bit.
Another update:
The test suite passes, except
t5000 - not ok 48 - archive and :(glob)

fatal: pathspec '*.abc' did not match any files

--
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: [PATCHv2] add: ignore only ignored files

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

 On Sun, Nov 23, 2014 at 10:10:47AM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  ... Possibly because I do not know that those instructions
  are written down anywhere. We usually catch such things in review these
  days, but there are many inconsistent spots in the existing suite.
 
 t/README has this
 
 Don't:
 
  - use '! git cmd' when you want to make sure the git command exits
with failure in a controlled way by calling die().  Instead,
use 'test_must_fail git cmd'.  This will signal a failure if git
dies in an unexpected way (e.g. segfault).
 
On the other hand, don't use test_must_fail for running regular
platform commands; just use '! cmd'.

 Thanks, I did not actually look and relied on my memory, which was
 obviously wrong. I agree that the instructions there are sufficient.

 Do we refer to t/README from CodingGuidelines where we tell the
 developers to always write tests to prevent other people from
 breaking tomorrow what you did today?  If not, perhaps that is what
 needs to be added.

 That might make sense. It might also be that Torsten simply overlooked
 it when asking his question (i.e., there is nothing to fix,
 documentation is not always read completely, and we can move on).

We actually do not have a reference to it anywhere.  For now, this
should suffice.

 Documentation/SubmittingPatches | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index fa71b5f..a3861a6 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -57,7 +57,8 @@ change, the approach taken by the change, and if relevant how 
this
 differs substantially from the prior version, are all good things
 to have.
 
-Make sure that you have tests for the bug you are fixing.
+Make sure that you have tests for the bug you are fixing.  See
+t/README for guidance of writing tests.
 
 When adding a new feature, make sure that you have new tests to show
 the feature triggers the new behaviour when it should, and to show the
--
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 RFC] CYGWIN: avoid implicit declaration warning

2014-11-24 Thread Jonathan Nieder

Torsten Bögershausen wrote:

 gcc under cygwin reports several warnings like this:

  warning: implicit declaration of function 'memmem'
   [-Wimplicit-function-declaration]

 This has been observed under CYGWIN-32 with GCC 4.7.3 as well
 as CYGWIN-64 with gcc v4.8.3-5 x86-64

 Do not #define _XOPEN_SOURCE 600 for CYGWIN.

 Reported-by: Ramsay Jones ram...@ramsay1.demon.co.uk
 Signed-off-by: Torsten Bögershausen tbo...@web.de
 ---
 This may be a start for a patch, tested under CYGWIN-32,
 both Windows7 and XP

The tested under part would also be a good addition to the commit
message.

  git-compat-util.h | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

Patch looks good to me.  Do you know if this has been reported to the
Cygwin maintainers?  The behavior seems counterintuitive --- I would
expect _GNU_SOURCE to override everything else (since I thought that
was the point of _GNU_SOURCE).

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] send-email: handle adjacent RFC 2047-encoded words properly

2014-11-24 Thread Роман Донченко

Junio C Hamano gits...@pobox.com писал в своём письме Mon, 24 Nov 2014  
10:27:51 +0300:


On Sun, Nov 23, 2014 at 3:50 PM, Роман Донченко d...@corrigendum.ru  
wrote:

The RFC says that they are to be concatenated after decoding (i.e. the
intervening whitespace is ignored).

I change the sender's name to an all-Cyrillic string in the tests so  
that

its encoded form goes over the 76 characters in a line limit, forcing
format-patch to split it into multiple encoded words.

Since I have to modify the regular expression for an encoded word  
anyway,

I take the opportunity to bring it closer to the spec, most notably
disallowing embedded spaces and making it case-insensitive (thus  
allowing

the encoding to be specified as both q and Q).

Signed-off-by: Роман Донченко d...@corrigendum.ru


This sounds like a worthy thing to do in general.

I wonder if the C implementation we have for mailinfo needs similar
update, though. I vaguely recall that we have case-insensitive start for
q/b segments, but do not remember the details offhand.


That's what git am uses, right? I think that already works correctly (or  
at least doesn't have the bug this patch fixes). I didn't do extensive  
testing or look at the code, though.




Was the change to the test to use Cyrillic really necessary, or did it
suffice if you simply extended the existsing Funny Name spelled with
strange accents, but you substituted the whole string anyway?

Until I found out what the new string says by running web-based
translation on it, I felt somewhat uneasy. As I do not read
Cyrillic/Russian, we may have been adding some profanity without
knowing. It turns out that the string just says Cyrillic Name, so I am
not against using the new string, but it simply looked odd to replace the
string whole-sale when you merely need a longer string. It made it look
as if a bug was specific to Cyrillic when it wasn't.


Ah, if only I had thought of including profanity beforehand. ;-)

Seriously though, I just needed to hit the 76 character limit, and  
switching the keyboard layout is a lot easier than copypasting Latin  
letters with diacritics (plus I had trouble coming up with a long enough  
extension of Funny Name...). I can see how that's problematic, though;  
I'll change it.



As you may notice by reading git log --no-merges from recent history,
we tend not to say I did X, I did Y. If the tone of the above message
were more similar to them, it may have been easier to read.


Technically, I said I do, not I did... but sure, point taken.

Roman.
--
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: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?

2014-11-24 Thread Nico Williams

Is there a plan for upgrading to a better hash function in the future?
 (E.g., should it become an urgent need.)

What are the roadblocks to adoption of a replacement hash function?
Just documenting this would go a long way towards making it possible
to upgrade some day.

Thanks,

Nico
--
--
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] send-email: handle adjacent RFC 2047-encoded words properly

2014-11-24 Thread Роман Донченко

Jeff King p...@peff.net писал в своём письме Mon, 24 Nov 2014 18:36:09  
+0300:



On Mon, Nov 24, 2014 at 02:50:04AM +0300, Роман Донченко wrote:


The RFC says that they are to be concatenated after decoding (i.e. the
intervening whitespace is ignored).

I change the sender's name to an all-Cyrillic string in the tests so  
that

its encoded form goes over the 76 characters in a line limit, forcing
format-patch to split it into multiple encoded words.

Since I have to modify the regular expression for an encoded word  
anyway,

I take the opportunity to bring it closer to the spec, most notably
disallowing embedded spaces and making it case-insensitive (thus  
allowing

the encoding to be specified as both q and Q).


The overall goal makes sense to me. Thanks for working on this. I have a
few questions/comments, though.


 sub unquote_rfc2047 {
local ($_) = @_;
+
+   my $et = qr/[!-@-~]+/; # encoded-text from RFC 2047
+   my $sep = qr/[ \t]+/;
+   my $encoded_word = qr/=\?($et)\?q\?($et)\?=/i;


The first $et in $encoded_word is actually the charset, which is defined
by RFC 2047 as:

 charset = token; see section 3

 token = 1*Any CHAR except SPACE, CTLs, and especials

 especials = ( / ) /  /  / @ / , / ; / : / 
/ / / [ / ] / ? / . / =

Your regex is a little more liberal. I doubt that it is a big deal in
practice (actually, in practice, I suspect [a-zA-Z0-9-] would be fine).
But if we are tightening things up in general, it may make sense to do
so here (and I notice that is_rfc2047_quoted does a more thorough $token
definition, and it probably makes sense for the two functions to be
consistent).


Yeah, I did realize that token is more restrictive than encoded-text, but  
I didn't want to stray too far from the subject line of the patch. What  
I'll probably do is split the patch into two, one for regex tweaking and  
one for multiple-word handling. And yeah, I'll try to make the two  
functions use the same regexes.




For your definition of encoded-text, RFC 2047 says:

 encoded-text = 1*Any printable ASCII character other than ?
  or SPACE

It looks like you pulled the definition of $et from is_rfc2047_quoted,
but I am not clear on where that original came from (it is from a3a8262,
but that commit message does not explain the regex).


No, it's actually an independent discovery. :-) I don't think it needs  
explanation, though - it's just a character class with two ranges covering  
every printable character but the question mark.



Also, I note that we handle 'q'-style encodings here, but not 'b'. I
wonder if it is worth adding that in while we are in the area (it is not
a big deal if you always send-email git-generated patches, as we never
generate it).


I could add b decoding, but since format-patch never generates b  
encodings, testing would be a problem. And I'd rather not do it without  
any tests.





+   s{$encoded_word(?:$sep$encoded_word)+}{


If I am reading this right, it requires at least two $encoded_words.
Should this + be a *?


I hang my head in shame. Looks like I'll have to add more tests...




+   my @words = split $sep, $;
+   foreach (@words) {
+   m/$encoded_word/;
+   $encoding = $1;
+   $_ = $2;
+   s/_/ /g;
+   s/=([0-9A-F]{2})/chr(hex($1))/eg;


In the spirit of your earlier change, should this final regex be
case-insensitive? RFC 2047 says only Upper case should be used for
hexadecimal digits A through F. but that does not seem like a MUST
to me.


Sounds reasonable.

Roman.
--
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] t5000 on Windows: do not mistake sh.exe as sh

2014-11-24 Thread Johannes Sixt

In their effort to emulate POSIX as close as possible, the MSYS tools
and Cygwin treat the file name foo.exe as foo when the latter is
asked for, but not present, but the former is present.

Following this rule, 'cp /bin/sh a/bin' actually copies the file
/bin/sh.exe, so that we now have a/bin/sh.exe in the repository. This
difference did not matter in the tests in the past because we were only
interested in the equality of contents generated in various ways. But
recently added tests check file names, in particular, the presence of
a/bin/sh. This test fails on Windows, as we do not have a file by this
name, but a/bin/sh.exe.

Use test-genrandom to generate the large binary file in the repository
under the expected name.

We could change the guilty line to 'cat /bin/sh a/bin/sh', but it is
better for test reproducibility to ensure that the test data is the same
across platforms, which test-genrandom can guarantee.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
Am 24.11.2014 um 17:00 schrieb Torsten Bögershausen:
 The test suite passes, except
 t5000 - not ok 48 - archive and :(glob)
 
 fatal: pathspec '*.abc' did not match any files

I've this patch in my tree for a while.

 t/t5000-tar-tree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index d01bbdc..4b68bba 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -101,7 +101,7 @@ test_expect_success \
  ten=0123456789  hundred=$ten$ten$ten$ten$ten$ten$ten$ten$ten$ten 
  echo long filename a/four$hundred 
  mkdir a/bin 
- cp /bin/sh a/bin 
+ test-genrandom frotz 50 a/bin/sh 
  printf A\$Format:%s\$O $SUBSTFORMAT a/substfile1 
  printf A not substituted O a/substfile2 
  if test_have_prereq SYMLINKS; then
-- 
2.0.0.12.gbcf935e

--
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 RFC] CYGWIN: avoid implicit declaration warning

 
 Patch looks good to me.  Do you know if this has been reported to the
 Cygwin maintainers?  The behavior seems counterintuitive --- I would
 expect _GNU_SOURCE to override everything else (since I thought that
 was the point of _GNU_SOURCE).
I don't know, it seems that CYGWIN is now in class with some BSD and Mac OS X.
Actually Ramsay did send a patch, so that this is obsolete now.


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

Re: git merge a b when a == b but neither == o is always a successful merge?

2014-11-24 Thread Daniel Hagerty

  I agree that the approach taken here is a sensible way to implement
  the design, _if_ the design and the problem it tries to solve makes
  sense.  I am not sure about that yet myself, though.

This is a first things first.

What aspect of the problem to be solved is in question?

From here, it seems obvious that there is textual data where the
 supression of sha1 identical files as non-conflicting isn't always
 proper.  I'm somewhat doubtful that the workflow design is really the
 problem -- I either have to remove a required feature of the broader
 design, or I have to move the problem somewhere else that isn't as
 well suited to handle it.  git seems the right tool for the job, but
 for the parameterization of merge.

I'd be happy for some route that doesn't involve changing upstream
git, but it doesn't sound like that exists.

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] change contract between system_path and it's callers

0xAX kuleshovm...@gmail.com writes:

 Now system_path returns path which is allocated string to callers;
 It prevents memory leaks in some places. All callers of system_path
 are owners of path string and they must release it.

 Signed-off-by: Alexander Kuleshov kuleshovm...@gmail.com
 ---

 -static const char *git_etc_gitattributes(void)
 +static char *git_etc_gitattributes(void)

Hmph, I think this should keep returning const char *, as the
caller is not expected to free the pointer or write into the memory
held by the returned string.  The same applies to the change of the
type in struct git_config_source.

The change in the semantics of system_path() made the get once and
keep returning it this function does safer and correct, but this
function itself did not change any behaviour from its caller's point
of view.

  {
 - static const char *system_wide;
 + static char *system_wide;
   if (!system_wide)
   system_wide = system_path(ETC_GITATTRIBUTES);
   return system_wide;


 @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)
   attr_stack = elem;
  
   if (git_attr_system()) {
 - elem = read_attr_from_file(git_etc_gitattributes(), 1);
 + char *etc_attributes = git_etc_gitattributes();
 + elem = read_attr_from_file(etc_attributes, 1);
 + free(etc_attributes);

And freeing here is actively wrong, I think.  You are freeing the
piece of memory still pointed by static char *system_wide in the
function git_etc_gitattributes(); when it is called again, the
caller will get a pointer into the memory you have freed here.

 diff --git a/builtin/config.c b/builtin/config.c
 index 15a7bea..266d42b 100644
 --- a/builtin/config.c
 +++ b/builtin/config.c
 @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char 
 *prefix)
   if (given_config_source.blob)
   die(editing blobs is not supported);
   git_config(git_default_config, NULL);
 - config_file = xstrdup(given_config_source.file ?
 -   given_config_source.file : 
 git_path(config));
 +
 + if (use_system_config)
 + config_file = given_config_source.file ?
 + 
 given_config_source.file : git_path(config);
 + else
 + config_file = xstrdup(given_config_source.file ?
 +   
 given_config_source.file : git_path(config));
 +

Sorry, but I do not understand this change.

Why do you need if use-system-config, do not allocate here and
then later everywhere if use-system-config, free it?  I would
understand if the change were earlier we had mixture of borrowed
and owned strings, but we make sure we always own the string by
running xstrdup() in the caller when we used to let this function
borrow, so that we can always free() before returning from here,
or something.

For example, in the original code, use_local_config codepath uses
git_pathdup(), which is an allocated piece of memory, to initialize
given_config_source.file.  Doesn't it need to be freed?

Even if it is not in the use_system_config mode, if
given_config_source.file is not an absolute path, we store a copy of
prefix-filename result to given_config_source.file.  Doesn't it need
to be freed?

I think the implementation strategy you took makes the result
unnecessarily messy and error prone.  Let's step back a bit.

When you have code that sometimes borrows memory and sometimes owns
memory, writing the clean-up part like this is error prone:

char *var;

if (A)
var = borrowed memory;
else if (B)
var = borrowed memory;
else if (C)
var = xstrdup(borrowed memory);
else
var = borrowed memory;

use var; /* a loong piece of code in reality */

if (C)
free(var);

because the set-up part can and will change over time as the code
evolves.  If written this way:

char *var, *to_free = NULL;

if (A)
var = borrowed memory;
else if (B)
var = borrowed memory;
else if (C)
to_free = var = xstrdup(borrowed memory);
else
var = borrowed memory;

use var; /* a loong piece of code in reality */

free(to_free);

the clean-up part would not have to worry how the set-up part
decided when to borrow memory and when to own memory.  Another way
to do so would obviously be:

char *var;

if (A)
var = xstrdup(borrowed memory);
else if (B)
var = xstrdup(borrowed memory);
else if (C)
var = xstrdup(borrowed memory);
else
var = xstrdup(borrowed memory);

use var; /* a loong piece of code in reality */

Re: [PATCH] t5000 on Windows: do not mistake sh.exe as sh

 
 diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
 index d01bbdc..4b68bba 100755
 --- a/t/t5000-tar-tree.sh
 +++ b/t/t5000-tar-tree.sh
 @@ -101,7 +101,7 @@ test_expect_success \
   ten=0123456789  hundred=$ten$ten$ten$ten$ten$ten$ten$ten$ten$ten 
   echo long filename a/four$hundred 
   mkdir a/bin 
 - cp /bin/sh a/bin 
 + test-genrandom frotz 50 a/bin/sh 

That's a fast respose :-)
The patch works for me, 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: Re*: [PATCH] change contract between system_path and it's callers

2014-11-24 Thread Alex Kuleshov


Junio C Hamano gits...@pobox.com @ 2014-11-25 01:33 ALMT:

 -static const char *git_etc_gitattributes(void)
 +static char *git_etc_gitattributes(void)

 Hmph, I think this should keep returning const char *, as the
 caller is not expected to free the pointer or write into the memory
 held by the returned string.  The same applies to the change of the
 type in struct git_config_source.

 The change in the semantics of system_path() made the get once and
 keep returning it this function does safer and correct, but this
 function itself did not change any behaviour from its caller's point
 of view.

Understand, will fix it.


  {
 -static const char *system_wide;
 +static char *system_wide;
  if (!system_wide)
  system_wide = system_path(ETC_GITATTRIBUTES);
  return system_wide;


 @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)
  attr_stack = elem;

  if (git_attr_system()) {
 -elem = read_attr_from_file(git_etc_gitattributes(), 1);
 +char *etc_attributes = git_etc_gitattributes();
 +elem = read_attr_from_file(etc_attributes, 1);
 +free(etc_attributes);

 And freeing here is actively wrong, I think.  You are freeing the
 piece of memory still pointed by static char *system_wide in the
 function git_etc_gitattributes(); when it is called again, the
 caller will get a pointer into the memory you have freed here.

Why? If i understand correctly we don't use etc_attributes anymore in
this function and if we'll call this function again
git_etc_gitattributes will create new pointer and system_path alloc
memory for it or i'm wrong with it?


 diff --git a/builtin/config.c b/builtin/config.c
 index 15a7bea..266d42b 100644
 --- a/builtin/config.c
 +++ b/builtin/config.c
 @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char 
 *prefix)
  if (given_config_source.blob)
  die(editing blobs is not supported);
  git_config(git_default_config, NULL);
 -config_file = xstrdup(given_config_source.file ?
 -  given_config_source.file : 
 git_path(config));
 +
 +if (use_system_config)
 +config_file = given_config_source.file ?
 +
 given_config_source.file : git_path(config);
 +else
 +config_file = xstrdup(given_config_source.file ?
 +  
 given_config_source.file : git_path(config));
 +

 Sorry, but I do not understand this change.

 Why do you need if use-system-config, do not allocate here and
 then later everywhere if use-system-config, free it?  I would
 understand if the change were earlier we had mixture of borrowed
 and owned strings, but we make sure we always own the string by
 running xstrdup() in the caller when we used to let this function
 borrow, so that we can always free() before returning from here,
 or something.

 For example, in the original code, use_local_config codepath uses
 git_pathdup(), which is an allocated piece of memory, to initialize
 given_config_source.file.  Doesn't it need to be freed?

 Even if it is not in the use_system_config mode, if
 given_config_source.file is not an absolute path, we store a copy of
 prefix-filename result to given_config_source.file.  Doesn't it need
 to be freed?

 I think the implementation strategy you took makes the result
 unnecessarily messy and error prone.  Let's step back a bit.

 When you have code that sometimes borrows memory and sometimes owns
 memory, writing the clean-up part like this is error prone:

   char *var;

   if (A)
   var = borrowed memory;
   else if (B)
   var = borrowed memory;
   else if (C)
   var = xstrdup(borrowed memory);
   else
   var = borrowed memory;

   use var; /* a loong piece of code in reality */

   if (C)
   free(var);

 because the set-up part can and will change over time as the code
 evolves.  If written this way:

   char *var, *to_free = NULL;

   if (A)
   var = borrowed memory;
   else if (B)
   var = borrowed memory;
   else if (C)
   to_free = var = xstrdup(borrowed memory);
   else
   var = borrowed memory;

   use var; /* a loong piece of code in reality */

   free(to_free);

 the clean-up part would not have to worry how the set-up part
 decided when to borrow memory and when to own memory.  Another way
 to do so would obviously be:

   char *var;

   if (A)
   var = xstrdup(borrowed memory);
   else if (B)
   var = xstrdup(borrowed memory);
   else if (C)
   var = xstrdup(borrowed memory);
   else
   var = xstrdup(borrowed memory);

   use var; /* a loong piece of code in reality */

Re: Re*: [PATCH] change contract between system_path and it's callers

Alex Kuleshov kuleshovm...@gmail.com writes:

 Junio C Hamano gits...@pobox.com @ 2014-11-25 01:33 ALMT:
 ...
 if (git_attr_system()) {
 -   elem = read_attr_from_file(git_etc_gitattributes(), 1);
 +   char *etc_attributes = git_etc_gitattributes();
 +   elem = read_attr_from_file(etc_attributes, 1);
 +   free(etc_attributes);

 And freeing here is actively wrong, I think.  You are freeing the
 piece of memory still pointed by static char *system_wide in the
 function git_etc_gitattributes(); when it is called again, the
 caller will get a pointer into the memory you have freed here.

 Why? If i understand correctly we don't use etc_attributes anymore in
 this function and if we'll call this function again
 git_etc_gitattributes will create new pointer and system_path alloc
 memory for it or i'm wrong with it?

The function keeps a singleton in static const char *system_wide
so that it has to call system_path() only once, and keeps the value
for second and subsequent calls.  From its callers' point of view,
they are only peeking the memory it returns.

This aspect does not change with your patch.

-static const char *git_etc_gitattributes(void)
+static char *git_etc_gitattributes(void)
 {
-   static const char *system_wide;
+   static char *system_wide;
if (!system_wide)
system_wide = system_path(ETC_GITATTRIBUTES);
return system_wide;

--
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: [PATCHv2] add: ignore only ignored files

On 2014-11-24 18.41, Junio C Hamano wrote:
...
 Do we refer to t/README from CodingGuidelines where we tell the
 developers to always write tests to prevent other people from
 breaking tomorrow what you did today?  If not, perhaps that is what
 needs to be added.

 That might make sense. It might also be that Torsten simply overlooked
 it when asking his question (i.e., there is nothing to fix,
 documentation is not always read completely, and we can move on).

Thanks, until yesterday I was unaware of t/README, but now I am :-)

--
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: Re*: [PATCH] change contract between system_path and it's callers

Alex Kuleshov kuleshovm...@gmail.com writes:

 One thing to note is that this illustration does not consider memory
 pointed at by the system_wide variable here (from attr.c)

 static const char *git_etc_gitattributes(void)
 {
 static const char *system_wide;
 if (!system_wide)
 system_wide = system_path(ETC_GITATTRIBUTES);
 return system_wide;
 }

 at the point of process exit as a leak.

 But why? We allocated memory to system_wide with system_path, next git
 will exit somewhere with die, but system_wide didn't free... Or i'm
 wrong here too?

It is in the same league as static const char *git_dir and friends
that appear in the file-scope-static of environment.c.  Keeping small
things around to be cleaned up by exit() is not a crime.
--
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 71/71] Change 'usually' to 'by default'

2014-11-24 Thread Richard Littauer

I've been confused several times by the docs when running across
the word 'usually'. It is difficult to know before hand if it
means 'under normal conditions' or 'by default'. I've gone through
the Documentation to identify specific cases where I think that 'by
default' is more explanatory than 'usually'. I know that patches
that touch too much code is unwelcome - as this is a small fix to
very specific lines, I think this might be OK.

For the majority of cases, usually works fine. However, especially
for the docs concerning flags, it is good to know if 'usually' was
just used by the documenter to mean 'unless your version has been
exntensively modified' or if it mean unless you've messed up.
Hopefully, this isn't too pedantic.

Signed-off-by: Richard Littauer richard.litta...@gmail.com
---
 Documentation/git-branch.txt  |  2 +-
 Documentation/git-cherry-pick.txt |  2 +-
 Documentation/git-commit.txt  |  2 +-
 Documentation/git-grep.txt|  2 +-
 Documentation/git-http-push.txt   |  2 +-
 Documentation/git-ls-files.txt|  2 +-
 Documentation/git-ls-remote.txt   |  2 +-
 Documentation/git-mailinfo.txt|  2 +-
 Documentation/git-pack-refs.txt   |  2 +-
 Documentation/git-push.txt|  4 ++--
 Documentation/git-read-tree.txt   |  6 +++---
 Documentation/git-rev-parse.txt   |  4 ++--
 Documentation/git-revert.txt  |  2 +-
 Documentation/git-send-pack.txt   |  2 +-
 Documentation/git-show-branch.txt |  2 +-
 Documentation/git-unpack-objects.txt  |  2 +-
 Documentation/gitattributes.txt   |  2 +-
 Documentation/gitcli.txt  |  6 +++---
 Documentation/gitweb.conf.txt | 10 +-
 Documentation/technical/api-parse-options.txt |  2 +-
 Documentation/technical/racy-git.txt  |  4 ++--
 Documentation/user-manual.txt |  9 -
 builtin/apply.c   |  2 +-
 contrib/hooks/multimail/README|  2 +-
 gitweb/INSTALL|  8 
 remote.c  |  2 +-
 remote.h  |  2 +-
 t/README  |  2 +-
 28 files changed, 45 insertions(+), 46 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 311b336..c5284e5 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -88,7 +88,7 @@ OPTIONS
Create the branch's reflog.  This activates recording of
all changes made to the branch ref, enabling use of date
based sha1 expressions such as branchname@\{yesterday}.
-   Note that in non-bare repositories, reflogs are usually
+   Note that in non-bare repositories, reflogs are
enabled by default by the `core.logallrefupdates` config option.
 
 -f::
diff --git a/Documentation/git-cherry-pick.txt 
b/Documentation/git-cherry-pick.txt
index 1c03c79..6d8a43e 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -86,7 +86,7 @@ OPTIONS
 
 -n::
 --no-commit::
-   Usually the command automatically creates a sequence of commits.
+   By default the command automatically creates a sequence of commits.
This flag applies the changes necessary to cherry-pick
each named commit to your working tree and the index,
without making any commit.  In addition, when this
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0bbc8f5..d82add3 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -203,7 +203,7 @@ variable (see linkgit:git-config[1]).
 -e::
 --edit::
The message taken from file with `-F`, command line with
-   `-m`, and from commit object with `-C` are usually used as
+   `-m`, and from commit object with `-C` are by default used as
the commit log message unmodified. This option lets you
further edit the message taken from these sources.
 
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 31811f1..f6b0638 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -124,7 +124,7 @@ OPTIONS
line.
 
 --full-name::
-   When run from a subdirectory, the command usually
+   When run from a subdirectory, the command by default
outputs paths relative to the current directory.  This
option forces paths to be output relative to the project
top directory.
diff --git a/Documentation/git-http-push.txt b/Documentation/git-http-push.txt
index 2e67362..bd72f75 100644
--- a/Documentation/git-http-push.txt
+++ b/Documentation/git-http-push.txt
@@ -28,7 +28,7 @@ OPTIONS
ref's history exist in the remote repository.
 
 --force::
-   Usually, the command refuses to update a

[PATCH 3/3] string_list: Remove string_list_insert_at_index from its API

This function behaves the same as string_list_insert, just the
starting indexes for searching, where to insert into the list is also
a parameter. So if you have knowledge on where to search for the string
to be inserted, you may have a speed up version of string_list_insert.

As we're not using this function throughout the codebase, get rid of it.

Signed-off-by: Stefan Beller sbel...@google.com
---
 string-list.c | 8 +---
 string-list.h | 2 --
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/string-list.c b/string-list.c
index c5aa076..9584fa6 100644
--- a/string-list.c
+++ b/string-list.c
@@ -59,13 +59,7 @@ static int add_entry(int insert_at, struct string_list 
*list, const char *string
 
 struct string_list_item *string_list_insert(struct string_list *list, const 
char *string)
 {
-   return string_list_insert_at_index(list, -1, string);
-}
-
-struct string_list_item *string_list_insert_at_index(struct string_list *list,
-int insert_at, const char 
*string)
-{
-   int index = add_entry(insert_at, list, string);
+   int index = add_entry(-1, list, string);
 
if (index  0)
index = -1 - index;
diff --git a/string-list.h b/string-list.h
index 40ffe0c..ee9b100 100644
--- a/string-list.h
+++ b/string-list.h
@@ -61,8 +61,6 @@ int string_list_find_insert_index(const struct string_list 
*list, const char *st
  * Returns the string_list_item, the string is part of.
  */
 struct string_list_item *string_list_insert(struct string_list *list, const 
char *string);
-struct string_list_item *string_list_insert_at_index(struct string_list *list,
-int insert_at, const char 
*string);
 
 /*
  * Checks if the given string is part of a sorted list. If it is part of the 
list,
-- 
2.2.0.rc3

--
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] string_list: document string_list_(insert,lookup)

Signed-off-by: Stefan Beller sbel...@google.com
---
 string-list.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/string-list.h b/string-list.h
index 494eb5d..40ffe0c 100644
--- a/string-list.h
+++ b/string-list.h
@@ -55,9 +55,19 @@ void string_list_remove_empty_items(struct string_list 
*list, int free_util);
 int string_list_has_string(const struct string_list *list, const char *string);
 int string_list_find_insert_index(const struct string_list *list, const char 
*string,
  int negative_existing_index);
+/*
+ * Inserts the given string into the sorted list.
+ * If the string already exists, the list is not altered.
+ * Returns the string_list_item, the string is part of.
+ */
 struct string_list_item *string_list_insert(struct string_list *list, const 
char *string);
 struct string_list_item *string_list_insert_at_index(struct string_list *list,
 int insert_at, const char 
*string);
+
+/*
+ * Checks if the given string is part of a sorted list. If it is part of the 
list,
+ * return the coresponding string_list_item, NULL otherwise.
+ */
 struct string_list_item *string_list_lookup(struct string_list *list, const 
char *string);
 
 /*
-- 
2.2.0.rc3

--
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] mailmap: use higher level string list functions

No functional changes intended. This commit makes user of higher level
and better documented functions of the string list API, so the code is
more understandable.

Note that also the required computational amount should not change
in principal as we need to look up the item no matter if it is already
part of the list or not. Once looked up, insertion comes for free.

Signed-off-by: Stefan Beller sbel...@google.com
---
 mailmap.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 81890a6..f0df350 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -78,15 +78,10 @@ static void add_mapping(struct string_list *map,
new_email = NULL;
}
 
-   if ((index = string_list_find_insert_index(map, old_email, 1))  0) {
-   /* mailmap entry exists, invert index value */
-   index = -1 - index;
-   me = (struct mailmap_entry *)map-items[index].util;
+   struct string_list_item *item = string_list_insert(map, old_email);
+   if (item-util) {
+   me = (struct mailmap_entry *)item-util;
} else {
-   /* create mailmap entry */
-   struct string_list_item *item;
-
-   item = string_list_insert_at_index(map, index, old_email);
me = xcalloc(1, sizeof(struct mailmap_entry));
me-namemap.strdup_strings = 1;
me-namemap.cmp = namemap_cmp;
-- 
2.2.0.rc3

--
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] refs.c: move reflog updates into its own function

Anything holding this back?

On Thu, Nov 20, 2014 at 4:38 PM, Stefan Beller sbel...@google.com wrote:
 From: Ronnie Sahlberg sahlb...@google.com

 write_ref_sha1 tries to update the reflog while updating the ref.
 Move these reflog changes out into its own function so that we can do the
 same thing if we write a sha1 ref differently, for example by writing a ref
 to the packed refs file instead.

 No functional changes intended. We only move some code out into a separate
 function.

 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 Signed-off-by: Stefan Beller sbel...@google.com
 ---

  Examining the refs-transaction-reflog series a bit closer, this seems to be
  one of the last independant patches, which make sense to rip out on a 
 per-patch
  basis.


  refs.c | 60 +++-
  1 file changed, 35 insertions(+), 25 deletions(-)

 diff --git a/refs.c b/refs.c
 index 005eb18..6837367 100644
 --- a/refs.c
 +++ b/refs.c
 @@ -3043,6 +3043,40 @@ int is_branch(const char *refname)
 return !strcmp(refname, HEAD) || starts_with(refname, 
 refs/heads/);
  }

 +static int write_sha1_update_reflog(struct ref_lock *lock,
 +   const unsigned char *sha1, const char *logmsg)
 +{
 +   if (log_ref_write(lock-ref_name, lock-old_sha1, sha1, logmsg)  0 ||
 +   (strcmp(lock-ref_name, lock-orig_ref_name) 
 +log_ref_write(lock-orig_ref_name, lock-old_sha1, sha1, logmsg) 
  0)) {
 +   unlock_ref(lock);
 +   return -1;
 +   }
 +   if (strcmp(lock-orig_ref_name, HEAD) != 0) {
 +   /*
 +* Special hack: If a branch is updated directly and HEAD
 +* points to it (may happen on the remote side of a push
 +* for example) then logically the HEAD reflog should be
 +* updated too.
 +* A generic solution implies reverse symref information,
 +* but finding all symrefs pointing to the given branch
 +* would be rather costly for this rare event (the direct
 +* update of a branch) to be worth it.  So let's cheat and
 +* check with HEAD only which should cover 99% of all usage
 +* scenarios (even 100% of the default ones).
 +*/
 +   unsigned char head_sha1[20];
 +   int head_flag;
 +   const char *head_ref;
 +   head_ref = resolve_ref_unsafe(HEAD, RESOLVE_REF_READING,
 + head_sha1, head_flag);
 +   if (head_ref  (head_flag  REF_ISSYMREF) 
 +   !strcmp(head_ref, lock-ref_name))
 +   log_ref_write(HEAD, lock-old_sha1, sha1, logmsg);
 +   }
 +   return 0;
 +}
 +
  /*
   * Write sha1 into the ref specified by the lock. Make sure that errno
   * is sane on error.
 @@ -3086,34 +3120,10 @@ static int write_ref_sha1(struct ref_lock *lock,
 return -1;
 }
 clear_loose_ref_cache(ref_cache);
 -   if (log_ref_write(lock-ref_name, lock-old_sha1, sha1, logmsg)  0 ||
 -   (strcmp(lock-ref_name, lock-orig_ref_name) 
 -log_ref_write(lock-orig_ref_name, lock-old_sha1, sha1, logmsg) 
  0)) {
 +   if (write_sha1_update_reflog(lock, sha1, logmsg)) {
 unlock_ref(lock);
 return -1;
 }
 -   if (strcmp(lock-orig_ref_name, HEAD) != 0) {
 -   /*
 -* Special hack: If a branch is updated directly and HEAD
 -* points to it (may happen on the remote side of a push
 -* for example) then logically the HEAD reflog should be
 -* updated too.
 -* A generic solution implies reverse symref information,
 -* but finding all symrefs pointing to the given branch
 -* would be rather costly for this rare event (the direct
 -* update of a branch) to be worth it.  So let's cheat and
 -* check with HEAD only which should cover 99% of all usage
 -* scenarios (even 100% of the default ones).
 -*/
 -   unsigned char head_sha1[20];
 -   int head_flag;
 -   const char *head_ref;
 -   head_ref = resolve_ref_unsafe(HEAD, RESOLVE_REF_READING,
 - head_sha1, head_flag);
 -   if (head_ref  (head_flag  REF_ISSYMREF) 
 -   !strcmp(head_ref, lock-ref_name))
 -   log_ref_write(HEAD, lock-old_sha1, sha1, logmsg);
 -   }
 if (commit_ref(lock)) {
 error(Couldn't set %s, lock-ref_name);
 unlock_ref(lock);
 --
 2.2.0.rc2.23.gca0107e

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

Re: [PATCH RFC] CYGWIN: avoid implicit declaration warning

On 24/11/14 07:20, Torsten Bögershausen wrote:
 On 2014-11-24 00.15, Ramsay Jones wrote:
 On 23/11/14 18:53, Junio C Hamano wrote:
 Ramsay Jones ram...@ramsay1.demon.co.uk writes:

 On 23/11/14 14:16, Torsten Bögershausen wrote:
 gcc under cygwin reports several warnings like this:
  warning: implicit declaration of function 'memmem'
   [-Wimplicit-function-declaration]
 This has been observed under CYGWIN-32 with GCC 4.7.3 as well
 as CYGWIN-64 with gcc v4.8.3-5 x86-64

 Heh, thanks for looking into this. Your email came at a good time,
 since I was just about to boot my old laptop into windows XP to
 test my patch on 32-bit cygwin! (If I had not been watching the
 F1 Grand Prix on TV, I would already have done so! ;-) ).

 It's been a while since I updated my 32-bit cygwin installation
 (about 6 months) but I'm a little surprised you found this issue
 with gcc 4.7.3 (I'm _almost_ tempted to boot that laptop anyway
 just to see what versions of software it is running).

 So you have an old installation to check how well the patched
 version is accepted by the old set of header files?

 ... I can, indeed, use this old installation to test this on
 32-bit cygwin. sigh, I thought I had dodged that bullet! ;-)

 
 It depends what we mean with old:
 cygwin 1.5 is old, and I lost my test installation this summer:

I updated from cygwin 1.5 to cygwin 1.7 at the beginning of the year.
Since it is no longer supported, I don't think we need to worry about
version 1.5. When I said 'old installation' I meant my old version 1.7
32-bit installation.

 One netbook was converted from XP to Linux, the other machine needs to be
 re-installed and CYGWIN 1.5 is no longer available for download.
 
 I can confirm that Ramsays patch works with CYGWIN 1.7 32 Bit.

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: [PATCH RFC] CYGWIN: avoid implicit declaration warning

On 24/11/14 17:59, Jonathan Nieder wrote:
 Torsten Bögershausen wrote:
 
 gcc under cygwin reports several warnings like this:

  warning: implicit declaration of function 'memmem'
   [-Wimplicit-function-declaration]

 This has been observed under CYGWIN-32 with GCC 4.7.3 as well
 as CYGWIN-64 with gcc v4.8.3-5 x86-64

 Do not #define _XOPEN_SOURCE 600 for CYGWIN.

 Reported-by: Ramsay Jones ram...@ramsay1.demon.co.uk
 Signed-off-by: Torsten Bögershausen tbo...@web.de
 ---
 This may be a start for a patch, tested under CYGWIN-32,
 both Windows7 and XP
 
 The tested under part would also be a good addition to the commit
 message.
 
  git-compat-util.h | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
 
 Patch looks good to me.  Do you know if this has been reported to the
 Cygwin maintainers?

As I said in an earlier email, I have searched the cygwin mailing list
(a few days ago now), but this issue had not been mentioned (modulo my
poor list searching fu!).

However, since I don't want to subscribe to (yet another) busy mailing
list, I won't be reporting this issue there. (If someone on this list
is already subscribed to that list, then ... ;-) ).

   The behavior seems counterintuitive --- I would
 expect _GNU_SOURCE to override everything else (since I thought that
 was the point of _GNU_SOURCE).

I had that impression too, but I need to read some more first.

Also, my theory about the cause of the problem has changed slightly
this evening, after booting up my old laptop and looking at an old
32-bit cygwin installation.

Rather than a change to '/sys/cdefs.h' which changed the priority
of _XOPEN_SOURCE, it seems that it was the string.h header that
changed; those functions used to be declared unconditionally, whereas
the new headers have them within preprocessor conditionals.

ATB,
Ramsay Jones



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

Re: [PATCH RFC] CYGWIN: avoid implicit declaration warning

Ramsay Jones ram...@ramsay1.demon.co.uk writes:

 I updated from cygwin 1.5 to cygwin 1.7 at the beginning of the year.
 Since it is no longer supported, I don't think we need to worry about
 version 1.5. When I said 'old installation' I meant my old version 1.7
 32-bit installation.

 One netbook was converted from XP to Linux, the other machine needs to be
 re-installed and CYGWIN 1.5 is no longer available for download.
 
 I can confirm that Ramsays patch works with CYGWIN 1.7 32 Bit.

 Thanks!

Thanks.  So the unconditional version of the patch is good to go, I
take?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

ping?

I asked around and the people who know of `git remote` fell in these two
categories:

 - those who know of this bug and then first set the fetch URL and
   then the push URL.
 - those who did not expect the current behavior.

The command 'git remote set-url NAME URL' reads as set the URL(s) for
remote NAME to URL. Currently it means set the fetch (and push) URL of
remote NAME to URL (depending on whether pushurl is set).

I propose to add the option --fetch next to --push with the meaning set
the fetch/push URL of remote NAME to URL. Then --fetch --push means
set the fetch and push URL of remote NAME to URL. In a future git
version, this could be made the default option to avoid surprises (which
would be backwards incompatible though).

As for the changelog entry,

The git remote set-url command now allows you to change just the
fetch URL without modifying the push URL using the new --fetch
option. For symmetry with the --push option.

(symmetry in the eyes of the user, not how it is implemented in the
git config.)

Opinions?

On Wednesday 19 November 2014 22:28:35 Peter Wu wrote:
 On Wednesday 19 November 2014 13:18:56 Junio C Hamano wrote:
  Junio C Hamano gits...@pobox.com writes:
   Jeff King p...@peff.net writes:
   If you are fetching from somebody else and then pushing into your
   own publishing repository (i.e. fork of that upstream), why isn't
   the sequence of event like this, instead?
  
   $ git clone $upstream
   $ browser github.com
   ... fork upstream to your own publishing repository ...
   $ git remote set-url --push mine url for your publish repo
  
   Isn't this one of those bad workflows encouraged by GitHub, for
   which you guys have to be punished ;-)?
 
 For forks, it usually goes like this:
 
 git clone $upstream
 ... realizes that is has a bug which I want to fix ...
 ... creates a new repo ...
 git remote rename origin upstream
 git remote add origin git@$personal_repo
 # --fetch is what I need
 git remote add --fetch https://$personal_repo
 
 I often start by entering/copying the ssh URL which is what I need for
 pushing. Later ssh-agent forget about my key and I realize that push
 works fine over https, so would like to set that... only to observe that
 is not possible in an straightforward way through 'git remote'.
 
  Coming back to the topic, how common would this oops, I cloned via
  a wrong transport be?  I am not opposed to giving a recovery method
  for gotcha that does not happen very often, but if such an addition
  adds undue confusion factor for people who use set-url for more
  common cases, that would be a bad trade-off.
 
 Well, people rarely need to use 'git remote' except when, well, they
 need to modify the remotes. Where does the confusion come from? I might
 be biased now that I know the internals. Maybe the https/ssh case above
 needs to be mentioned in the documentation? What do you think of the
 updated documentation by the way?

--
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] refs.c: move reflog updates into its own function

Stefan Beller sbel...@google.com writes:

 Anything holding this back?

I do not recall anything objectionable offhand.  Perhaps fell
between the cracks during a patch storm or something.
--
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 v2] mailmap: use higher level string list functions

No functional changes intended. This commit makes user of higher level
and better documented functions of the string list API, so the code is
more understandable.

Note that also the required computational amount should not change
in principal as we need to look up the item no matter if it is already
part of the list or not. Once looked up, insertion comes for free.

Signed-off-by: Stefan Beller sbel...@google.com
---

Changes since Version 1:
* Remove declaration-after-statement.

 mailmap.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 81890a6..3b00a65 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -71,6 +71,7 @@ static void add_mapping(struct string_list *map,
char *old_name, char *old_email)
 {
struct mailmap_entry *me;
+   struct string_list_item *item;
int index;
 
if (old_email == NULL) {
@@ -78,15 +79,10 @@ static void add_mapping(struct string_list *map,
new_email = NULL;
}
 
-   if ((index = string_list_find_insert_index(map, old_email, 1))  0) {
-   /* mailmap entry exists, invert index value */
-   index = -1 - index;
-   me = (struct mailmap_entry *)map-items[index].util;
+   item = string_list_insert(map, old_email);
+   if (item-util) {
+   me = (struct mailmap_entry *)item-util;
} else {
-   /* create mailmap entry */
-   struct string_list_item *item;
-
-   item = string_list_insert_at_index(map, index, old_email);
me = xcalloc(1, sizeof(struct mailmap_entry));
me-namemap.strdup_strings = 1;
me-namemap.cmp = namemap_cmp;
-- 
2.2.0.rc3

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

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Peter Wu pe...@lekensteyn.nl writes:

 I propose to add the option --fetch next to --push with the meaning set
 the fetch/push URL of remote NAME to URL. Then --fetch --push means
 set the fetch and push URL of remote NAME to URL. 

What would (and should) the configuration look like after you did
this?

git remote set-url nick $url1
git remote set-url nick --push $url2
git remote set-url nick $url3

Whatever happens without your patch after the above is what the
current users (i.e. those who do not use the --fetch option) expect,
so if the behaviour does not change with your patch, then there is
one less incompatibilities to worry about.

A new option --fetch introducing a different behaviour is
perfectly fine; existing users who are not using it will not be
harmed by sudden behaviour change.

 In a future git
 version, this could be made the default option to avoid surprises (which
 would be backwards incompatible though).

I am not sure what you mean by default.  If you mean set both if
remote.nick.pushurl does not exist but otherwise update only
remote.nick.url, then the sequence

git remote set-url nick $url1
git remote set-url nick --push $url2
git remote set-url nick $url3

would retain the current behaviour, so it probably is OK.

If you mean to always set remote.nick.url and remote.nick.pushurl
pointing at the same value when neither --fetch nor --push is given,
That would make the sequence behave quite different from what people
would expect, and you would need to devise a transition plan to
first start warning when the user did something that will behave
differently between the current version and the future version
without changing the behaviour, then switch the behaviour but keep
warning and finally remove the warning, or something like that.

And the above three-command sequence may not be the only case where
the change you are proposing may hurt existing users.

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

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

On Monday 24 November 2014 14:04:07 Junio C Hamano wrote:
 Peter Wu pe...@lekensteyn.nl writes:
 
  I propose to add the option --fetch next to --push with the meaning set
  the fetch/push URL of remote NAME to URL. Then --fetch --push means
  set the fetch and push URL of remote NAME to URL. 
 
 What would (and should) the configuration look like after you did
 this?
 
   git remote set-url nick $url1
 git remote set-url nick --push $url2
 git remote set-url nick $url3
 
 Whatever happens without your patch after the above is what the
 current users (i.e. those who do not use the --fetch option) expect,
 so if the behaviour does not change with your patch, then there is
 one less incompatibilities to worry about.
 
 A new option --fetch introducing a different behaviour is
 perfectly fine; existing users who are not using it will not be
 harmed by sudden behaviour change.

As stated before, I took care to avoid backwards incompatibilities. The
command will still work as expected by the users who are aware of this
particular behavior. What I am suggesting (and which is independent of
the patch) is to make the command have a more consistent behavior.
Either it should set the fetch URL, or both the fetch and push URL, but
not vary its behavior depending on whether a push URL is set or not.
That should make the behavior of the command more consistent.

  In a future git version, this could be made the default option to
  avoid surprises (which would be backwards incompatible though).
 
 I am not sure what you mean by default.  If you mean set both if
 remote.nick.pushurl does not exist but otherwise update only
 remote.nick.url, then the sequence
 
   git remote set-url nick $url1
 git remote set-url nick --push $url2
 git remote set-url nick $url3
 
 would retain the current behaviour, so it probably is OK.
 
 If you mean to always set remote.nick.url and remote.nick.pushurl
 pointing at the same value when neither --fetch nor --push is given,
 That would make the sequence behave quite different from what people
 would expect, and you would need to devise a transition plan to
 first start warning when the user did something that will behave
 differently between the current version and the future version
 without changing the behaviour, then switch the behaviour but keep
 warning and finally remove the warning, or something like that.
 
 And the above three-command sequence may not be the only case where
 the change you are proposing may hurt existing users.

The default refers to the behavior of git remote set-url in absence
of --push and --fetch options. A transition period is expected (if
this idea is put forward). Since nobody seems to be bitten by this
option, I am not sure if it really adds much value to make this change
though.
-- 
Kind regards,
Peter
https://lekensteyn.nl

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

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

On Mon, Nov 24, 2014 at 11:16:03PM +0100, Peter Wu wrote:

  A new option --fetch introducing a different behaviour is
  perfectly fine; existing users who are not using it will not be
  harmed by sudden behaviour change.
 
 As stated before, I took care to avoid backwards incompatibilities. The
 command will still work as expected by the users who are aware of this
 particular behavior.

Right. My original complaint was only that --fetch is not as
orthogonal to --push (and an optionless set-url) as it could be. I
think the alternatives for going forward are basically:

  1. Name it something besides --fetch (but that's rather clunky).

  2. Migrate to new behavior, which is what is being discussed here.
 Probably needs a transition period?

  3. Live with it. Probably address the weirdness in the documentation.

  4. Do nothing, drop the patch.

I think I'd be OK with (3), with an appropriate documentation update.

-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 RFC] CYGWIN: avoid implicit declaration warning

On 23/11/14 23:15, Ramsay Jones wrote:
On 23/11/14 18:53, Junio C Hamano wrote:
Ramsay Jones ram...@ramsay1.demon.co.uk writes:

On 23/11/14 14:16, Torsten Bögershausen wrote:
gcc under cygwin reports several warnings like this:
warning: implicit declaration of function 'memmem'
[-Wimplicit-function-declaration]
This has been observed under CYGWIN-32 with GCC 4.7.3 as well
as CYGWIN-64 with gcc v4.8.3-5 x86-64

Heh, thanks for looking into this. Your email came at a good time,
since I was just about to boot my old laptop into windows XP to
test my patch on 32-bit cygwin! (If I had not been watching the
F1 Grand Prix on TV, I would already have done so! ;-) ).

It's been a while since I updated my 32-bit cygwin installation
(about 6 months) but I'm a little surprised you found this issue
with gcc 4.7.3 (I'm _almost_ tempted to boot that laptop anyway
just to see what versions of software it is running).

So you have an old installation to check how well the patched
version is accepted by the old set of header files?

... I can, indeed, use this old installation to test this on
32-bit cygwin. sigh, I thought I had dodged that bullet! ;-)

[I can't do this tonight and I'm pretty busy tomorrow, so it
may have to wait until tomorrow evening at the earliest. sorry
about that. :( ]

OK, so I had a look tonight and, as I told Jonathan earlier, my
theory for the cause of the issue has changed slightly.

First, I simply re-built git to make sure this issue wasn't present
(I would have been shocked if it was, but ...) and it was fine.
The master branch was at commit 7b69fcb (aka Git 2.1.0-rc1).
I applied my patch to this commit and re-built, again with no
problem. I then fetched the current version of git (i.e. commit
652e759 aka Git 2.2.0-rc3) and re-built; no problem. Finally, after
applying my patch to this commit, once again the build was free
from problems.

Note that I have not run any tests (it takes far too long), simply
a full build (make clean; make).

Some other things to note:

$ uname -a
CYGWIN_NT-5.1 toshiba 1.7.30(0.272/5/3) 2014-05-23 10:36 i686 Cygwin

$ gcc --version
gcc (GCC) 4.8.3
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ ls -l /cygdrive/c/Documents\ and\ Settings/ramsay/Downloads/http%3a
%2f%2fmirrors.kernel.org%2fsourceware%2fcygwin%2f/x86/release/gcc/gcc-core/
total 38M
-rwx--+ 1 ramsay None 13M Nov 10 2013 gcc-core-4.8.2-1.tar.xz*
-rwx--+ 1 ramsay None 13M Dec 22 2013 gcc-core-4.8.2-2.tar.xz*
-rw-r--r-- 1 ramsay None 13M Jun 3 13:06 gcc-core-4.8.3-1.tar.xz
$ # ie this is the first build of the compiler package for v4.8.3

$ cygcheck -f /usr/include/string.h
cygwin-1.7.30-1
$ cygcheck -f /usr/include/sys/cdefs.h
cygwin-1.7.30-1

$ ls /usr/lib/gcc/i686-pc-cygwin/4.8.3/include-fixed/
limits.h README syslimits.h

While on my (new) 64-bit installation:

$ uname -a
CYGWIN_NT-6.3 satellite 1.7.33-2(0.280/5/3) 2014-11-13 15:47 x86_64 Cygwin
$ gcc --version
gcc (GCC) 4.8.3
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ ls -l
/cygdrive/c/Users/ramsay/Downloads/http%3a%2f%2fmirrors.kernel.org%2fsourceware%2fcygwin%2f/x86_64/release/gcc/gcc-core/
total 66M
-rw-r--r-- 1 ramsay None 13M May 18 2014 gcc-core-4.8.2-3.tar.xz
-rw-r--r-- 1 ramsay None 13M Jun 7 00:06 gcc-core-4.8.3-2.tar.xz
-rw-r--r-- 1 ramsay None 14M Aug 30 22:48 gcc-core-4.8.3-3.tar.xz
-rw-r--r-- 1 ramsay None 14M Nov 12 20:19 gcc-core-4.8.3-4.tar.xz
-rw-r--r-- 1 ramsay None 14M Nov 20 13:07 gcc-core-4.8.3-5.tar.xz
$ # ie this is the fifth build of the compiler package for v4.8.3

$ cygcheck -f /usr/include/string.h
cygwin-devel-1.7.33-1
$ cygcheck -f /usr/include/sys/cdefs.h
cygwin-devel-1.7.33-1
$ cygcheck -f /usr/lib/gcc/x86_64-pc-cygwin/4.8.3/include-fixed/sys/cdefs.h
gcc-core-4.8.3-5
$

Although I have not done an actual diff of the various cdef.h files, they
do appear to be more or less the same. In other words, I no longer think
that the change results from a 'change in priority of _XOPEN_SOURCE'. The
issue is simply that in the old string.h header these functions were
declared unconditionally; in the new headers the are contained within
preprocessor conditionals using the __GNU_VISIBLE and __BSD_VISIBLE macros
which are not set when _XOPEN_SOURCE is set (despite _GNU_SOURCE and
_BSD_SOURCE being set).

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: [RFC] [PATCH] remote: add new --fetch option for set-url

On Monday 24 November 2014 17:22:44 Jeff King wrote:
 On Mon, Nov 24, 2014 at 11:16:03PM +0100, Peter Wu wrote:
 
   A new option --fetch introducing a different behaviour is
   perfectly fine; existing users who are not using it will not be
   harmed by sudden behaviour change.
  
  As stated before, I took care to avoid backwards incompatibilities. The
  command will still work as expected by the users who are aware of this
  particular behavior.
 
 Right. My original complaint was only that --fetch is not as
 orthogonal to --push (and an optionless set-url) as it could be. I
 think the alternatives for going forward are basically:
 
   1. Name it something besides --fetch (but that's rather clunky).

It is not orthogonal to --push in the config, but the behavior exposed
to the user is orthogonal unless I am missing something?

I can understand that --fetch sounds a bit weird, what about this
natural translation:

git remote: set the URL (only the fetch one) for NAME to URL
git remote set-url --only=fetch NAME URL

git remote: set the URL (only the push one) for NAME to URL
git remote set-url --only=push NAME URL
(obsoletes --push)

git remote: set the URL (both) for NAME to URL
git remote set-url --only=both NAME URL
(it would be nice if --only=both (weird!) can be removed in the
future such that the option is more natural)

git remote: set the URL for NAME to URL
git remote set-url NAME URL
(current behavior: YOU git guru knows what I do right?)


   2. Migrate to new behavior, which is what is being discussed here.
  Probably needs a transition period?

A transition period would also help to solicit feedback.

   3. Live with it. Probably address the weirdness in the documentation.
 
   4. Do nothing, drop the patch.
 
 I think I'd be OK with (3), with an appropriate documentation update.

I prefer 1 for now as it avoids the extra manual action I have to take
when changing URLs.

Peter

--
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 RFC] CYGWIN: avoid implicit declaration warning

Ramsay Jones ram...@ramsay1.demon.co.uk writes:

 ...
 Although I have not done an actual diff of the various cdef.h files, they
 do appear to be more or less the same. In other words, I no longer think
 that the change results from a 'change in priority of _XOPEN_SOURCE'. The
 issue is simply that in the old string.h header these functions were
 declared unconditionally; in the new headers the are contained within
 preprocessor conditionals using the __GNU_VISIBLE and __BSD_VISIBLE macros
 which are not set when _XOPEN_SOURCE is set (despite _GNU_SOURCE and
 _BSD_SOURCE being set).

So I can take your version [*1*], drop this bit from the log:

This seems to be caused by a change to the system headers which
results in the _XOPEN_SOURCE macro now having prioity over the
_GNU_SOURCE and _BSD_SOURCE macros (they are simply ignored).
This in turn leads to the declarations of the above functions
to be suppressed.

and replace it with something like:

string.h on Cygwin used to always declare the above functions,
but a recent version of it no longer make them visible when
_XOPEN_SOURCE is set (even if _GNU_SOURCE and _BSD_SOURCE is
set).

and keep the rest, I think.

Thanks for digging this thoroughly.


[Reference]

*1* http://article.gmane.org/gmane.comp.version-control.git/260091


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

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

On Mon, Nov 24, 2014 at 11:47:30PM +0100, Peter Wu wrote:

  Right. My original complaint was only that --fetch is not as
  orthogonal to --push (and an optionless set-url) as it could be. I
  think the alternatives for going forward are basically:
  
1. Name it something besides --fetch (but that's rather clunky).
 
 It is not orthogonal to --push in the config, but the behavior exposed
 to the user is orthogonal unless I am missing something?

My complaint is that you have three possible options to provide: --push,
--fetch, or no option at all. And --fetch sometimes behaves like no
option, and sometimes not. Which is the confusing/non-orthogonal part.

 I can understand that --fetch sounds a bit weird, what about this
 natural translation:
 
 git remote: set the URL (only the fetch one) for NAME to URL
 git remote set-url --only=fetch NAME URL
 
 git remote: set the URL (only the push one) for NAME to URL
 git remote set-url --only=push NAME URL
 (obsoletes --push)
 
 git remote: set the URL (both) for NAME to URL
 git remote set-url --only=both NAME URL
 (it would be nice if --only=both (weird!) can be removed in the
 future such that the option is more natural)
 
 git remote: set the URL for NAME to URL
 git remote set-url NAME URL
 (current behavior: YOU git guru knows what I do right?)

Yeah, I think that addresses my concern (because it explicitly leaves
no-option as a historical curiosity, and not as an implicit version of
--both).

3. Live with it. Probably address the weirdness in the documentation.
  
4. Do nothing, drop the patch.
  
  I think I'd be OK with (3), with an appropriate documentation update.
 
 I prefer 1 for now as it avoids the extra manual action I have to take
 when changing URLs.

I'm not sure if I was clear on (3), but live with it was live with
your original patch. Which I think you would also be happy with.

-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 RFC] CYGWIN: avoid implicit declaration warning

On 24/11/14 21:44, Junio C Hamano wrote:
 Ramsay Jones ram...@ramsay1.demon.co.uk writes:
 
 I updated from cygwin 1.5 to cygwin 1.7 at the beginning of the year.
 Since it is no longer supported, I don't think we need to worry about
 version 1.5. When I said 'old installation' I meant my old version 1.7
 32-bit installation.

 One netbook was converted from XP to Linux, the other machine needs to be
 re-installed and CYGWIN 1.5 is no longer available for download.

 I can confirm that Ramsays patch works with CYGWIN 1.7 32 Bit.

 Thanks!
 
 Thanks.  So the unconditional version of the patch is good to go, I
 take?
 

Hmm, I don't know what you mean by 'unconditional version'. ;-)

Anyway, the commit message of my patch needs some edits to reflect
my new 'theory' of the cause. I suppose I should try to track down
the changes to the cygwin headers to be more confident that I have
actually identified the correct cause. (Like Jonathan, I'm still a
bit surprised that _GNU_SOURCE doesn't trump _XOPEN_SOURCE, but I
can't say that it is a bug).

However, I suspect that the patch as is, modulo commit message edits,
should be sufficient to fix this up (at least for now).

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

[PATCH] git-sh-setup.sh: use dashdash with basename call

2014-11-24 Thread Dan Wyand

Calling basename on a argument that starts with a dash, like a login shell,
will result in an error. This patch adds '--' before the argument so that the
argument is interpreted properly.
---
 git-sh-setup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 9447980..5cdae33 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -81,7 +81,7 @@ if test -n $OPTIONS_SPEC; then
echo exit $?
)
 else
-   dashless=$(basename $0 | sed -e 's/-/ /')
+   dashless=$(basename -- $0 | sed -e 's/-/ /')
usage() {
die usage: $dashless $USAGE
}
-- 
2.1.3

--
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] send-email: handle adjacent RFC 2047-encoded words properly

On Mon, Nov 24, 2014 at 09:26:22PM +0300, Роман Донченко wrote:

 Yeah, I did realize that token is more restrictive than encoded-text, but I
 didn't want to stray too far from the subject line of the patch. What I'll
 probably do is split the patch into two, one for regex tweaking and one for
 multiple-word handling. And yeah, I'll try to make the two functions use the
 same regexes.

Thanks, I think that sounds like a good plan.

 For your definition of encoded-text, RFC 2047 says:
 
  encoded-text = 1*Any printable ASCII character other than ?
   or SPACE
 
 It looks like you pulled the definition of $et from is_rfc2047_quoted,
 but I am not clear on where that original came from (it is from a3a8262,
 but that commit message does not explain the regex).
 
 No, it's actually an independent discovery. :-) I don't think it needs
 explanation, though - it's just a character class with two ranges covering
 every printable character but the question mark.

And now it is my turn to hang my head in shame. I viewed that as a set
of characters, rather than ranges. The - just blended into the mass of
punctuation, and I mistook the ! for negation.

I wonder if it would be more readable as:

  [\x21-\x3e\x40-\x7e]

or something. I guess perl even has classes pre-made for printable
ascii. I dunno. It may be OK as-is, too, and I just need to read more
carefully. :)

 Also, I note that we handle 'q'-style encodings here, but not 'b'. I
 wonder if it is worth adding that in while we are in the area (it is not
 a big deal if you always send-email git-generated patches, as we never
 generate it).
 
 I could add b decoding, but since format-patch never generates b
 encodings, testing would be a problem. And I'd rather not do it without any
 tests.

I think you could include some literal fixtures in the test suite (t5100
already does this for mailinfo). But I don't think handling 'b' is a
requirement here. It's really orthogonal to your patches, and nobody has
actually asked for it, so I don't mind leaving it for another day.

-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 RFC] CYGWIN: avoid implicit declaration warning

On 24/11/14 22:50, Junio C Hamano wrote:
 Ramsay Jones ram...@ramsay1.demon.co.uk writes:
 
 ...
 Although I have not done an actual diff of the various cdef.h files, they
 do appear to be more or less the same. In other words, I no longer think
 that the change results from a 'change in priority of _XOPEN_SOURCE'. The
 issue is simply that in the old string.h header these functions were
 declared unconditionally; in the new headers the are contained within
 preprocessor conditionals using the __GNU_VISIBLE and __BSD_VISIBLE macros
 which are not set when _XOPEN_SOURCE is set (despite _GNU_SOURCE and
 _BSD_SOURCE being set).
 
 So I can take your version [*1*], drop this bit from the log:
 
 This seems to be caused by a change to the system headers which
 results in the _XOPEN_SOURCE macro now having prioity over the
 _GNU_SOURCE and _BSD_SOURCE macros (they are simply ignored).
 This in turn leads to the declarations of the above functions
 to be suppressed.
 
 and replace it with something like:
 
 string.h on Cygwin used to always declare the above functions,
 but a recent version of it no longer make them visible when
 _XOPEN_SOURCE is set (even if _GNU_SOURCE and _BSD_SOURCE is
 set).
 
 and keep the rest, I think.

Perfect! Thanks! (actually, another minor typo: the second warning
message, indent the ^ by two spaces - it should be pointing to the
= symbol).

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: [RFC] [PATCH] remote: add new --fetch option for set-url

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

 My complaint is that you have three possible options to provide: --push,
 --fetch, or no option at all. And --fetch sometimes behaves like no
 option, and sometimes not. Which is the confusing/non-orthogonal part.

 I can understand that --fetch sounds a bit weird, what about this
 natural translation:
 
 git remote: set the URL (only the fetch one) for NAME to URL
 git remote set-url --only=fetch NAME URL
 
 git remote: set the URL (only the push one) for NAME to URL
 git remote set-url --only=push NAME URL
 (obsoletes --push)
 
 git remote: set the URL (both) for NAME to URL
 git remote set-url --only=both NAME URL
 (it would be nice if --only=both (weird!) can be removed in the
 future such that the option is more natural)
 
 git remote: set the URL for NAME to URL
 git remote set-url NAME URL
 (current behavior: YOU git guru knows what I do right?)

 Yeah, I think that addresses my concern (because it explicitly leaves
 no-option as a historical curiosity, and not as an implicit version of
 --both).

Fine by me, too.
--
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] commit: inform pre-commit if --amend is used

2014-11-24 Thread Eric Sunshine

On Mon, Nov 24, 2014 at 6:21 AM, Øystein Walle oys...@gmail.com wrote:
 When a commit is amended a pre-commit hook that verifies the commit's
 contents might not find what it's looking for if for example it looks at
 the differences against HEAD when HEAD~1 might be more appropriate.
 Inform the commit hook that --amend is being used so that hook authors
 can do e.g.

 if test $1 = amend
 then
 ...
 else
 ...
 fi

 to handle these situations.

 Signed-off-by: Øystein Walle oys...@gmail.com
 ---
 diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
 index 984889b..be97676 100755
 --- a/t/t7503-pre-commit-hook.sh
 +++ b/t/t7503-pre-commit-hook.sh
 @@ -136,4 +136,18 @@ test_expect_success 'check the author in hook' '
 git show -s
  '

 +# a hook that checks if amend is passed as an argument
 +cat  $HOOK EOF
 +#!/bin/sh
 +test \$1 = amend
 +EOF

write_script would be the modern way to create this hook.

 +
 +test_expect_success 'check that amend argument is given' '
 +   git commit --amend --allow-empty
 +'
 +
 +test_expect_success 'check that amend argument is NOT given' '
 +   ! git commit --allow-empty

You probably want test_must_fail rather than '!' [1].

 +'
 +
  test_done
 --

[1]: http://thread.gmane.org/gmane.comp.version-control.git/259871/focus=260092
--
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-sh-setup.sh: use dashdash with basename call

2014-11-24 Thread Dan Wyand

Calling basename on a argument that starts with a dash, like a login shell,
will result in an error. This patch adds '--' before the argument so that the
argument is interpreted properly.

Signed-off-by: Dan Wyand danwy...@gmail.com
---
 git-sh-setup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 9447980..5cdae33 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -81,7 +81,7 @@ if test -n $OPTIONS_SPEC; then
echo exit $?
)
 else
-   dashless=$(basename $0 | sed -e 's/-/ /')
+   dashless=$(basename -- $0 | sed -e 's/-/ /')
usage() {
die usage: $dashless $USAGE
}
-- 
2.1.3

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

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

On Monday 24 November 2014 17:54:57 Jeff King wrote:
 On Mon, Nov 24, 2014 at 11:47:30PM +0100, Peter Wu wrote:
  I can understand that --fetch sounds a bit weird, what about this
  natural translation:
  
  git remote: set the URL (only the fetch one) for NAME to URL
  git remote set-url --only=fetch NAME URL
  
  git remote: set the URL (only the push one) for NAME to URL
  git remote set-url --only=push NAME URL
  (obsoletes --push)
  
  git remote: set the URL (both) for NAME to URL
  git remote set-url --only=both NAME URL
  (it would be nice if --only=both (weird!) can be removed in the
  future such that the option is more natural)
  
  git remote: set the URL for NAME to URL
  git remote set-url NAME URL
  (current behavior: YOU git guru knows what I do right?)
 
 Yeah, I think that addresses my concern (because it explicitly leaves
 no-option as a historical curiosity, and not as an implicit version of
 --both).

Ok, I will make a clear note about the default (without --only) behavior
having weird behavior for historical reasons. Are you really OK with
--only=both? It sounds a bit odd (mathematically speaking it is correct
as fetch and push are both partitions that form the whole set if you
ignore the historical behavior).

 3. Live with it. Probably address the weirdness in the documentation.
   
 4. Do nothing, drop the patch.
   
   I think I'd be OK with (3), with an appropriate documentation update.
  
  I prefer 1 for now as it avoids the extra manual action I have to take
  when changing URLs.
 
 I'm not sure if I was clear on (3), but live with it was live with
 your original patch. Which I think you would also be happy with.

Oh yes, I misunderstood this one ;)

What about the translations? Should I send a separate patch for that or
can I update all translations at once?
-- 
Kind regards,
Peter
https://lekensteyn.nl

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

color box, display box, corrugated box, color card, blister card, color sleeve, hang tag, label

2014-11-24 Thread Jinghao Printing - CHINA

Hi, this is David Wu from Shanghai, China.
We are a printing company, we can print color box, corrugated box,
label, hang tag etc.
Please let me know if you need these.

I will send you the website then.

Best regards,
David Wu
--
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

color box, display box, corrugated box, color card, blister card, color sleeve, hang tag, label

2014-11-24 Thread Jinghao Printing - CHINA

Hi, this is David Wu from Shanghai, China.
We are a printing company, we can print color box, corrugated box,
label, hang tag etc.
Please let me know if you need these.

I will send you the website then.

Best regards,
David Wu
--
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 3/3] string_list: Remove string_list_insert_at_index from its API

Stefan Beller sbel...@google.com writes:

 This function behaves the same as string_list_insert, just the
 starting indexes for searching, where to insert into the list is also
 a parameter. So if you have knowledge on where to search for the string
 to be inserted, you may have a speed up version of string_list_insert.

The last half-sentence I am having trouble parsing.  If you have
that knowledge where the item should go, you can insert it at the
right location faster than calling string_list_insert()?

That sounds like a useful feature to me.

But if nobody uses that feature, there is no point keeping it, no
matter how useful it may sound.  So,... I am not sure.

The function would be error prone if used on a string-list that is
accessed via string_list_insert() API, which makes it only useful if
the user of the API is in full control of the sort order, so I do
not mind removing it and won't be saying that is useful, we should
keep it and use it more.

But your log message confuses me


 As we're not using this function throughout the codebase, get rid of it.

 Signed-off-by: Stefan Beller sbel...@google.com
 ---
  string-list.c | 8 +---
  string-list.h | 2 --
  2 files changed, 1 insertion(+), 9 deletions(-)

 diff --git a/string-list.c b/string-list.c
 index c5aa076..9584fa6 100644
 --- a/string-list.c
 +++ b/string-list.c
 @@ -59,13 +59,7 @@ static int add_entry(int insert_at, struct string_list 
 *list, const char *string
  
  struct string_list_item *string_list_insert(struct string_list *list, const 
 char *string)
  {
 - return string_list_insert_at_index(list, -1, string);
 -}
 -
 -struct string_list_item *string_list_insert_at_index(struct string_list 
 *list,
 -  int insert_at, const char 
 *string)
 -{
 - int index = add_entry(insert_at, list, string);
 + int index = add_entry(-1, list, string);
  
   if (index  0)
   index = -1 - index;
 diff --git a/string-list.h b/string-list.h
 index 40ffe0c..ee9b100 100644
 --- a/string-list.h
 +++ b/string-list.h
 @@ -61,8 +61,6 @@ int string_list_find_insert_index(const struct string_list 
 *list, const char *st
   * Returns the string_list_item, the string is part of.
   */
  struct string_list_item *string_list_insert(struct string_list *list, const 
 char *string);
 -struct string_list_item *string_list_insert_at_index(struct string_list 
 *list,
 -  int insert_at, const char 
 *string);
  
  /*
   * Checks if the given string is part of a sorted list. If it is part of the 
 list,
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-sh-setup.sh: use dashdash with basename call

Dan Wyand danwy...@gmail.com writes:

 Calling basename on a argument that starts with a dash, like a login shell,
 will result in an error. This patch adds '--' before the argument so that the
 argument is interpreted properly.
 ---

Makes sense.

Please sign-off your patch (see Documentation/SubmittingPatches).

  git-sh-setup.sh | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/git-sh-setup.sh b/git-sh-setup.sh
 index 9447980..5cdae33 100644
 --- a/git-sh-setup.sh
 +++ b/git-sh-setup.sh
 @@ -81,7 +81,7 @@ if test -n $OPTIONS_SPEC; then
   echo exit $?
   )
  else
 - dashless=$(basename $0 | sed -e 's/-/ /')
 + dashless=$(basename -- $0 | sed -e 's/-/ /')
   usage() {
   die usage: $dashless $USAGE
   }
--
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

Our cumbersome mailing list workflow (was: Re: [PATCH 0/6] repack_without_refs(): convert to string_list)

2014-11-24 Thread Michael Haggerty

On 11/21/2014 07:00 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 I don't think that those iterations changed anything substantial that
 overlaps with my version, but TBH it's such a pain in the ass working
 with patches in email that I don't think I'll go to the effort of
 checking for sure unless somebody shows interest in actually using my
 version.

 Sorry for being grumpy today :-(
 
 Is the above meant as a grumpy rant to be ignored, or as a
 discussion starter to improve the colaboration to allow people to
 work better together instead of stepping on each other's patches?

I think I know the sentiments of the mailing list regulars well enough
that it didn't seem worthwhile to open this topic again, so I was just
letting off steam without any hope of changing anything. But since you
asked...

Let me list the aspects of our mailing list workflow that I find
cumbersome as a contributor and reviewer:

* Submitting patches to the mailing list is an ordeal of configuring
format-patch and send-email and getting everything just right, using
instructions that depend on the local environment. We saw that hardly
any GSoC applicants were able to get it right on their first attempt.
Submitting a patch series should be as simple as git push.

* Once patches are submitted, there is no assurance that you (Junio)
will apply them to your tree at the same point that the submitter
developed and tested them.

* The branch name that you choose for a patch series is not easily
derivable from the patches as they appeared in the mailing list. Trying
to figure out whether/where the patches exist in your tree is a largely
manual task. The reverse mapping, from in-tree commit to the email where
it was proposed, is even more difficult to infer.

* Your tree has no indication of which version of a patch series (v1,
v2, etc) is currently applied.

The previous three points combine to make it awkward to get patches into
my local repository to review or test. There are two alternatives, both
cumbersome and imprecise:

  * I do git fetch gitster, then try to figure out whether the branch
I'm interested in is present, what its name is, and whether the version
in your tree is the latest version, then git checkout xy/foobar.

  * Or I save the emails to a temporary directory (awkward because, Oh
Horror, I use Thunderbird and not mutt as email client), hope that I've
guessed the right place to apply them, run git am, and later try to
remember to clean up the temporary directory.

* Once I've done that, the supplemental comments from the emails (the
cover letter and the text under the ---) are nowhere available in the
Git repository. So if I want to see the changes in context plus the
supplemental comments, I have to jump back and forth between email
client and Git repo. Plus I have to jump around the rest of the email
thread to see what comments other reviewers have already made about the
series.

* Following patch series across iterations is also awkward. To compare
two versions, I have to first get both patch series into my repo, which
involves digging through the ML history to find older versions, followed
by the git am steps. Often submitters are nice enough to put links to
previous versions of their patch series in their cover letters, but the
links are to a web-based email archive, from which it is even more
awkward to grab and apply patches. So in practice I then go back to my
email client and search my local archive for my copy of the same email
that was referenced in the archive, and apply the patch from there.
Finding comments about old versions of a patch series is nearly as much
work.

* Because of the indeterminate application point, accumulating
Signed-off-by lines, changed committer metadata, and maintainer tweaks,
the commits that make it to the official tree have different SHA-1s than
the commits in the submitter's tree, and both are different than the
commits in the tree of any reviewer who got the patches using git am.
This makes it hard to be sure that everybody is on the same page. It
also makes it awkward for people to exchange ideas for further changes
via Git protocols in the form of patches.

* Because of the crude serialization of patches through email, it is
only possible to submit linear patch series, not merge commits.

Hmmm, I think that covers most of the problems of handling patches and
review via a mailing list.


What are some alternatives?

I did enjoy the variety of reviewing some patch series using Gerrit. It
is nice that it tracks the evolution of a patch from version to version,
and that the comments made on all versions of a patch are summarized in
a single place. This makes it easier to avoid commenting on issues that
other reviewers have already noted and easier to check that your own
comments have been addressed by later versions of the patch. On the
other hand, Gerrit seems strongly focused on individual patches rather
than on patch series (which might not match

Re: [PATCH 71/71] Change 'usually' to 'by default'

2014-11-24 Thread Eric Sunshine

On Mon, Nov 24, 2014 at 4:07 PM, Richard Littauer
richard.litta...@gmail.com wrote:
 I've been confused several times by the docs when running across
 the word 'usually'. It is difficult to know before hand if it
 means 'under normal conditions' or 'by default'. I've gone through
 the Documentation to identify specific cases where I think that 'by
 default' is more explanatory than 'usually'. I know that patches
 that touch too much code is unwelcome - as this is a small fix to
 very specific lines, I think this might be OK.

 For the majority of cases, usually works fine. However, especially
 for the docs concerning flags, it is good to know if 'usually' was
 just used by the documenter to mean 'unless your version has been
 exntensively modified' or if it mean unless you've messed up.
 Hopefully, this isn't too pedantic.

 Signed-off-by: Richard Littauer richard.litta...@gmail.com
 ---
 diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt
 index 154081f..c97e62a 100644
 --- a/Documentation/git-pack-refs.txt
 +++ b/Documentation/git-pack-refs.txt
 @@ -56,7 +56,7 @@ a repository with many branches of historical interests.

  --no-prune::

 -The command usually removes loose refs under `$GIT_DIR/refs`
 +The command be default removes loose refs under `$GIT_DIR/refs`

s/be/by/

  hierarchy after packing them.  This option tells it not to.


 diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
 index ebe7a6c..dc07b64 100644
 --- a/Documentation/gitweb.conf.txt
 +++ b/Documentation/gitweb.conf.txt
 @@ -222,7 +222,7 @@ The values of these variables are paths on the filesystem.

  $GIT::
 Core git executable to use.  By default set to `$GIT_BINDIR/git`, 
 which
 -   in turn is by default set to `$(bindir)/git`.  If you use Git 
 installed
 +   in turn is defaulted to `$(bindir)/git`.  If you use Git installed

This would read better as:

in turn defaults to ...

 from a binary package, you should usually set this to /usr/bin/git.
 This can just be git if your web server has a sensible PATH; from
 security point of view it is better to use absolute path to git 
 binary.
 @@ -633,10 +633,10 @@ override::
 overridable, which means that it can be configured
 (or enabled/disabled) on a per-repository basis.
  +
 -Usually given feature is configurable via the `gitweb.feature`
 +Usually the given feature is configurable via the `gitweb.feature`
  config variable in the per-repository Git configuration file.
  +
 -*Note* that no feature is overridable by default.
 +*Note* that no feature is overridable usually.

This reads strangely and seems more ambiguous than the original text.

  sub::
 Internal detail of implementation.  What is important is that
 diff --git a/Documentation/technical/racy-git.txt 
 b/Documentation/technical/racy-git.txt
 index 242a044..8c577ee 100644
 --- a/Documentation/technical/racy-git.txt
 +++ b/Documentation/technical/racy-git.txt
 @@ -38,10 +38,10 @@ files vs symbolic links) and executable bits (only for 
 regular
  files) from `st_mode` member, `st_mtime` and `st_ctime`
  timestamps, `st_uid`, `st_gid`, `st_ino`, and `st_size` members.
  With a `USE_STDEV` compile-time option, `st_dev` is also
 -compared, but this is not enabled by default because this member
 +compared, but this is not enabled usually because this member
  is not stable on network filesystems.  With `USE_NSEC`
  compile-time option, `st_mtim.tv_nsec` and `st_ctim.tv_nsec`
 -members are also compared, but this is not enabled by default
 +members are also compared, but this is not enabled usually

These reverse changes from by default to usually seem to make the
text more ambiguous.

In fact, all the reverse changes from by default to usually
throughout this patch make the text less clear. Your commit message
does not explain this decision.

  because in-core timestamps can have finer granularity than
  on-disk timestamps, resulting in meaningless changes when an
  inode is evicted from the inode cache.  See commit 8ce13b0 diff --git 
 a/gitweb/INSTALL b/gitweb/INSTALL
 index 408f285..23636b9 100644
 --- a/gitweb/INSTALL
 +++ b/gitweb/INSTALL
 @@ -140,7 +140,7 @@ You can specify the following configuration variables 
 when building GIT:
 gitweb.cgi script.  [No default]
   * GITWEB_HOME_LINK_STR
 String of the home link on top of all pages, leading to $home_link
 -   (usually main gitweb page, which means projects list).  Used as first
 +   (by default main gitweb page, which means projects list).  Used as first

In an earlier change in this patch, you inserted the word the to
improve grammar. Such a change could be applied here:

   (by default, the main ...

 part of gitweb view breadcrumb trail: home / project / view.
 [Default: projects]
   * GITWEB_SITENAME
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo

Re: [PATCH 3/3] string_list: Remove string_list_insert_at_index from its API

On Mon, Nov 24, 2014 at 4:25 PM, Junio C Hamano gits...@pobox.com wrote:
 Stefan Beller sbel...@google.com writes:

 This function behaves the same as string_list_insert, just the
 starting indexes for searching, where to insert into the list is also
 a parameter. So if you have knowledge on where to search for the string
 to be inserted, you may have a speed up version of string_list_insert.

 The last half-sentence I am having trouble parsing.  If you have
 that knowledge where the item should go, you can insert it at the
 right location faster than calling string_list_insert()?

 That sounds like a useful feature to me.

 But if nobody uses that feature, there is no point keeping it, no
 matter how useful it may sound.  So,... I am not sure.

 The function would be error prone if used on a string-list that is
 accessed via string_list_insert() API, which makes it only useful if
 the user of the API is in full control of the sort order, so I do
 not mind removing it and won't be saying that is useful, we should
 keep it and use it more.

 But your log message confuses me


Well, I'm sorry for the confusion, here is what I was trying to say:

We do not remove functionality from the API, but rather remove a
a highly optimized version of a function, which we are not using
after patch 2 of the series anyway.

We should keep it around only iff we strongly believe there will be
a use case later again. But for now I do have the opinion, we're
better off removing the function as a smaller API / header file
serves us better. Specially new comers may appreciate a smaller
API but documented there.

Maybe we can drop the commit message altogether if my thoughts
are still confusing?
--
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: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?

2014-11-24 Thread Duy Nguyen

On Tue, Nov 25, 2014 at 1:14 AM, Nico Williams n...@cryptonector.com wrote:
 Is there a plan for upgrading to a better hash function in the future?
  (E.g., should it become an urgent need.)

 What are the roadblocks to adoption of a replacement hash function?
 Just documenting this would go a long way towards making it possible
 to upgrade some day.

The biggest obstacle is the assumption of SHA-1 everywhere in the
source code (e.g. assume the object name always takes 20 bytes). Brian
started on cleaning that up [1] but I think it's stalled. Then we need
to deal with upgrade path for SHA-1 repos.

[1] http://thread.gmane.org/gmane.comp.version-control.git/248054
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?

2014-11-24 Thread Jonathan Nieder

Duy Nguyen wrote:

 The biggest obstacle is the assumption of SHA-1 everywhere in the
 source code (e.g. assume the object name always takes 20 bytes). Brian
 started on cleaning that up [1] but I think it's stalled. Then we need
 to deal with upgrade path for SHA-1 repos.

I think the biggest obstacle is the upgrade path. ;-)

If the upgrade path is taken care of, I won't mind writing and
reviewing a coccinelle-generated patch to replace 20, 40, 21, 41, and
so on with appropriate constants.  Or we can take the first 20 bytes
of a SHA-256, which is already supposed to have better security
properties than SHA-1.

Another obstacle is hard-coded SHA-1s in tests.

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

[PATCHv2 3/3] string_list: Remove string_list_insert_at_index from its API

Nobody is using this function any more, let's get rid of it.

Signed-off-by: Stefan Beller sbel...@google.com
---
 string-list.c | 8 +---
 string-list.h | 2 --
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/string-list.c b/string-list.c
index c5aa076..9584fa6 100644
--- a/string-list.c
+++ b/string-list.c
@@ -59,13 +59,7 @@ static int add_entry(int insert_at, struct string_list 
*list, const char *string
 
 struct string_list_item *string_list_insert(struct string_list *list, const 
char *string)
 {
-   return string_list_insert_at_index(list, -1, string);
-}
-
-struct string_list_item *string_list_insert_at_index(struct string_list *list,
-int insert_at, const char 
*string)
-{
-   int index = add_entry(insert_at, list, string);
+   int index = add_entry(-1, list, string);
 
if (index  0)
index = -1 - index;
diff --git a/string-list.h b/string-list.h
index 40ffe0c..ee9b100 100644
--- a/string-list.h
+++ b/string-list.h
@@ -61,8 +61,6 @@ int string_list_find_insert_index(const struct string_list 
*list, const char *st
  * Returns the string_list_item, the string is part of.
  */
 struct string_list_item *string_list_insert(struct string_list *list, const 
char *string);
-struct string_list_item *string_list_insert_at_index(struct string_list *list,
-int insert_at, const char 
*string);
 
 /*
  * Checks if the given string is part of a sorted list. If it is part of the 
list,
-- 
2.2.0.rc3

--
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: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?

2014-11-24 Thread Duy Nguyen

On Tue, Nov 25, 2014 at 8:23 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 I think the biggest obstacle is the upgrade path. ;-)

In the worst case we can always treat new repos as a different VCS. So
people will need a migration from SHA-1 to the new format, just like
they migrate from SVN/CVS to Git. Painful but simple.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3 v2] mailmap: use higher level string list functions

2014-11-24 Thread Eric Sunshine

On Mon, Nov 24, 2014 at 4:52 PM, Stefan Beller sbel...@google.com wrote:
 No functional changes intended. This commit makes user of higher level

s/user/use/

 and better documented functions of the string list API, so the code is
 more understandable.

 Note that also the required computational amount should not change
 in principal as we need to look up the item no matter if it is already
 part of the list or not. Once looked up, insertion comes for free.

 Signed-off-by: Stefan Beller sbel...@google.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: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?

That's not *as* painful, because you'd have the beautiful
fast-{import/export} tools in your new and old version control system.
But yeah, there might be better ways to do so.


On Mon, Nov 24, 2014 at 5:52 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Tue, Nov 25, 2014 at 8:23 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 I think the biggest obstacle is the upgrade path. ;-)

 In the worst case we can always treat new repos as a different VCS. So
 people will need a migration from SHA-1 to the new format, just like
 they migrate from SVN/CVS to Git. Painful but simple.
 --
 Duy
 --
 To unsubscribe from this list: send the line unsubscribe git in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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

[PATCHv3 2/3] mailmap: use higher level string list functions

No functional changes intended. This commit makes use of higher level
and better documented functions of the string list API, so the code is
more understandable.

Note that also the required computational amount should not change
in principal as we need to look up the item no matter if it is already
part of the list or not. Once looked up, insertion comes for free.

Signed-off-by: Stefan Beller sbel...@google.com
---
Changes since Version 1:
* Remove declaration-after-statement.

Changes Version 1 to Version 2:
* typo in commit message

 mailmap.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 81890a6..3b00a65 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -71,6 +71,7 @@ static void add_mapping(struct string_list *map,
char *old_name, char *old_email)
 {
struct mailmap_entry *me;
+   struct string_list_item *item;
int index;
 
if (old_email == NULL) {
@@ -78,15 +79,10 @@ static void add_mapping(struct string_list *map,
new_email = NULL;
}
 
-   if ((index = string_list_find_insert_index(map, old_email, 1))  0) {
-   /* mailmap entry exists, invert index value */
-   index = -1 - index;
-   me = (struct mailmap_entry *)map-items[index].util;
+   item = string_list_insert(map, old_email);
+   if (item-util) {
+   me = (struct mailmap_entry *)item-util;
} else {
-   /* create mailmap entry */
-   struct string_list_item *item;
-
-   item = string_list_insert_at_index(map, index, old_email);
me = xcalloc(1, sizeof(struct mailmap_entry));
me-namemap.strdup_strings = 1;
me-namemap.cmp = namemap_cmp;
-- 
2.2.0.rc3

--
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] commit: inform pre-commit if --amend is used

On Mon, Nov 24, 2014 at 12:21:51PM +0100, Øystein Walle wrote:

  This hook is invoked by 'git commit', and can be bypassed
 -with `--no-verify` option.  It takes no parameter, and is
 +with `--no-verify` option.  It takes one parameter which is amend if
 +`--amend` was used when committing and empty otherwise. It is

This interface change is OK for backwards compatibility, since existing
scripts would not look at the arguments (which are always empty
currently). And I think it is OK for adding new options in the future,
too, because the option is always present (just sometimes as an empty
string).  Can we make the non-amend option something more verbose than
the empty string (like noamend)? I have two reasons:

  1. It is a bit more obvious when debugging or dumping arguments (e.g.,
 via GIT_TRACE), especially if new options are added after the
 first.

  2. It makes it easier for a script to work on old and new versions of
 git. It sees either amend or noamend for the two obvious cases,
 and if it sees no argument, then it knows that it does not know
 either way (it is running on an old version of git).

 Technically one can tell the difference in shell between an empty
 string and a missing argument, but it is sufficiently subtle that I
 think noamend is a better route.

 +# a hook that checks if amend is passed as an argument
 +cat  $HOOK EOF
 +#!/bin/sh
 +test \$1 = amend
 +EOF

Eric mentioned write_script already (and it would be nice to convert the
existing uses in t7503 to use it). You may also want to use \EOF to
inhibit interpolation in the here-document, which means you do not have
to quote variables inside the script.

-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: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?

On Tue, Nov 25, 2014 at 08:52:58AM +0700, Duy Nguyen wrote:

 On Tue, Nov 25, 2014 at 8:23 AM, Jonathan Nieder jrnie...@gmail.com wrote:
  I think the biggest obstacle is the upgrade path. ;-)
 
 In the worst case we can always treat new repos as a different VCS. So
 people will need a migration from SHA-1 to the new format, just like
 they migrate from SVN/CVS to Git. Painful but simple.

Maybe we can fix the tree-sorting order while we are at it. :)

More seriously, there may come a day when we are ready to break
compatibility completely with a new Git v3.0 (2.0 is already taken, of
course). I do not have immediate plans for it, but it's possible that
multiple factors may make such a move desirable sometime in the next 10
years, and that would be a good time to jump hash algorithms, as well.

So it's possible that procrastinating on SHA-1 issues may be the least
painful route. Or it may just be pushing off the day of pain. :)

-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: BUG in http-backend.c http.receivepack

On Mon, Nov 24, 2014 at 09:18:52AM +0100, Springer, Stephan wrote:

 I found bug in http-backend.c with config-flag http.receivepack  You
 describe in our documentation: This serves git send-pack clients,
 allowing push. It is disabled by default for anonymous users, and
 enabled by default for users authenticated by the web server. It can
 be disabled by setting this item to false, or enabled for all users,
 including anonymous users, by setting it to true.
 That cannot work, while svc-enable less than 0. See attachment

Sorry, I don't quite understand. The enabled field is one of:

  -1: we allow access if $REMOTE_USER is set, and otherwise not
   0: we never allow access
   1: we always allow access

The default is -1. By setting it to true or false you get 1 or 0,
respectively. You cannot explicitly ask for the default, except by not
setting the value in the first place.

 #
 # better (svc-enabled = 0) than can ?REMOTE_USER? enable push 
 function 
 #
 if (svc-enabled  0) {
const char *user = getenv(REMOTE_USER);
svc-enabled = (user  *user) ? 1 : 0;
 }

If this condition were svc-enabled = 0, then setting the config
option to false, which should turn off access, will respect
$REMOTE_USER instead. That is not right.

Can you describe what you're configuring and running, what behavior you
expect, and what you get instead?

-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: [PATCHv2] add: ignore only ignored files

On Mon, Nov 24, 2014 at 09:41:00AM -0800, Junio C Hamano wrote:

 We actually do not have a reference to it anywhere.  For now, this
 should suffice.
 
  Documentation/SubmittingPatches | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
 index fa71b5f..a3861a6 100644
 --- a/Documentation/SubmittingPatches
 +++ b/Documentation/SubmittingPatches
 @@ -57,7 +57,8 @@ change, the approach taken by the change, and if relevant 
 how this
  differs substantially from the prior version, are all good things
  to have.
  
 -Make sure that you have tests for the bug you are fixing.
 +Make sure that you have tests for the bug you are fixing.  See
 +t/README for guidance of writing tests.

That looks a good improvement to me.

-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: [RFC] [PATCH] remote: add new --fetch option for set-url

On Tue, Nov 25, 2014 at 12:27:31AM +0100, Peter Wu wrote:

 On Monday 24 November 2014 17:54:57 Jeff King wrote:
  On Mon, Nov 24, 2014 at 11:47:30PM +0100, Peter Wu wrote:
   I can understand that --fetch sounds a bit weird, what about this
   natural translation:
   
   git remote: set the URL (only the fetch one) for NAME to URL
   git remote set-url --only=fetch NAME URL
   
   git remote: set the URL (only the push one) for NAME to URL
   git remote set-url --only=push NAME URL
   (obsoletes --push)
   
   git remote: set the URL (both) for NAME to URL
   git remote set-url --only=both NAME URL
   (it would be nice if --only=both (weird!) can be removed in the
   future such that the option is more natural)
   
   git remote: set the URL for NAME to URL
   git remote set-url NAME URL
   (current behavior: YOU git guru knows what I do right?)
  
  Yeah, I think that addresses my concern (because it explicitly leaves
  no-option as a historical curiosity, and not as an implicit version of
  --both).
 
 Ok, I will make a clear note about the default (without --only) behavior
 having weird behavior for historical reasons. Are you really OK with
 --only=both? It sounds a bit odd (mathematically speaking it is correct
 as fetch and push are both partitions that form the whole set if you
 ignore the historical behavior).

Maybe --operation={push,fetch,both} would be less odd (though
--operation is rather clunky, I could not think of a better word). It
is the conjunction of --only an both that makes little sense.

However, I think what removed the confusion for me in your --only=both
proposal was the presence of a both option, since it made it more
clear that is not what no-option means. So what about just --push,
--fetch, and --both? Explain the current behavior of no-options in
the documentation as a historical oddity.

That also gives us an easy path forward for changing the behavior.
During the transition period, people should use --push, --fetch, or
--both. Using no-options provides a warning. After a settling period,
the no-option behavior will switch to one of those (presumably --both),
and drop the warning.

You do not have to do the migration path if you don't want to. Adding
--fetch and --both scratches your itch and sets us up to migrate
later.

 What about the translations? Should I send a separate patch for that or
 can I update all translations at once?

You do not have to update the translations. When we near a release, the
l10n coordinator will run make pot to update po/git.pot with the
strings marked for translation, and then the translators will write
translations for the new strings. You are of course welcome to help with
the translation effort at that stage. :)

Details are in po/README.

-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: [RFC] [PATCH] remote: add new --fetch option for set-url

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

 However, I think what removed the confusion for me in your --only=both
 proposal was the presence of a both option, since it made it more
 clear that is not what no-option means. So what about just --push,
 --fetch, and --both?

I think that is the set that is most sensible, at least
syntactically, among the ones I heard so far in this thread.

However, one thing still makes me wonder

After doing set-url --fetch and nothing else, because the user
never said --both or --push, does the user get a configuration
where git push fails?  Or does set-url --fetch still gives us
remote.nick.url and causes git push to also go there?

If that is the case, then did addition of --fetch achieve anything
to reduce confusion?

After doing set-url --push and nothing else, I suspect that having
remote.nick.pushURL alone without remote.nick.URL will make git fetch
to fail, which would be in line with my expectation.  I just expected
anything we do in the name of symmetry or consistency would work the
same/symmetric way, I cannot see how set-url --fetch would work to
make its effect symmetric to the set-url --push one.

Puzzled...



--
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] commit: inform pre-commit if --amend is used

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

   1. It is a bit more obvious when debugging or dumping arguments (e.g.,
  via GIT_TRACE), especially if new options are added after the
  first.

   2. It makes it easier for a script to work on old and new versions of
  git. It sees either amend or noamend for the two obvious cases,
  and if it sees no argument, then it knows that it does not know
  either way (it is running on an old version of git).

  Technically one can tell the difference in shell between an empty
  string and a missing argument, but it is sufficiently subtle that I
  think noamend is a better route.

If we ever add more info, would we want to keep piling on new
arguments, though?  Wouldn't it a viable option to use amend vs
not giving anything (not even an empty string), so that normal case
there won't be no parameter?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

On Mon, Nov 24, 2014 at 08:55:13PM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  However, I think what removed the confusion for me in your --only=both
  proposal was the presence of a both option, since it made it more
  clear that is not what no-option means. So what about just --push,
  --fetch, and --both?
 
 I think that is the set that is most sensible, at least
 syntactically, among the ones I heard so far in this thread.
 
 However, one thing still makes me wonder
 
 After doing set-url --fetch and nothing else, because the user
 never said --both or --push, does the user get a configuration
 where git push fails?  Or does set-url --fetch still gives us
 remote.nick.url and causes git push to also go there?

I think the latter. And that makes sense to me. Push falls back to fetch
at runtime. And you never set a push url, so that's what happens.

But it is not symmetric. We do not fall back fetch to push. We _could_,
but that is a separate issue (one for git-fetch, and not git-remote).
And I do not think it is one anybody is particularly asking for.

We could also stop making push fall back to fetch. But I think people
would find that irritating.

I dunno. I think there has always been an implicit subordinate
relationship between fetch and push URLs, with fetch being the main
one. Maybe that is so ingrained in me at this point that I do not see a
problem with the asymmetry.

-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] commit: inform pre-commit if --amend is used