Re: [PATCH 60/68] prefer memcpy to strcpy

[René Scharfe]

Am 27.09.2015 um 15:06 schrieb Torsten Bögershausen:

On 2015-09-27 13.19, René Scharfe wrote:

Am 24.09.2015 um 23:08 schrieb Jeff King:

When we already know the length of a string (e.g., because
we just malloc'd to fit it), it's nicer to use memcpy than
strcpy, as it makes it more obvious that we are not going to
overflow the buffer (because the size we pass matches the
size in the allocation).

This also eliminates calls to strcpy, which make auditing
the code base harder.

Signed-off-by: Jeff King 
---
   compat/nedmalloc/nedmalloc.c | 5 +++--
   fast-import.c| 5 +++--
   revision.c   | 2 +-
   3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
index 609ebba..a0a16eb 100644
--- a/compat/nedmalloc/nedmalloc.c
+++ b/compat/nedmalloc/nedmalloc.c
@@ -957,8 +957,9 @@ char *strdup(const char *s1)
   {
   char *s2 = 0;
   if (s1) {
-s2 = malloc(strlen(s1) + 1);
-strcpy(s2, s1);
+size_t len = strlen(s1) + 1;
+s2 = malloc(len);
+memcpy(s2, s1, len);


This leaves the last byte uninitialized; it was set to NUL by strcpy() before.


len is == strlen() +1, which should cover the NUL:

1 byte extra for NUL is allocated,
and memcpy will copy NUL from source.
(Or do I miss somethong ?)


No, you're right.  Sorry for the noise.

I fully blame this on lack of coffeine because my electric kettle just 
broke. O_o


René
--
To unsubscribe from this list: send the line "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: Single brackets matching in .gitignore rules

[Andrey Loskutov]

On Sunday 27 September 2015 10:58 Andreas Schwab wrote:
> 
> ] by itself is not special in a glob.
> 
> Andreas.
> 

OK, this would explain the current state: once Git sees opened bracket, it 
always tries to parse the glob pattern and if this fails (if the bracket is not 
closed), the ignore rule does not match anything. This does not happen for 
closing bracket and therefore the difference in behavior.

Thanks!

P.S.
In case anyone is interested, here is the path for JGit: 
https://git.eclipse.org/r/#/c/56773/4

-- 
Kind regards,
google.com/+AndreyLoskutov
--
To unsubscribe from this list: send the line "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 60/68] prefer memcpy to strcpy

[Torsten Bögershausen]

On 2015-09-27 13.19, René Scharfe wrote:
> Am 24.09.2015 um 23:08 schrieb Jeff King:
>> When we already know the length of a string (e.g., because
>> we just malloc'd to fit it), it's nicer to use memcpy than
>> strcpy, as it makes it more obvious that we are not going to
>> overflow the buffer (because the size we pass matches the
>> size in the allocation).
>>
>> This also eliminates calls to strcpy, which make auditing
>> the code base harder.
>>
>> Signed-off-by: Jeff King 
>> ---
>>   compat/nedmalloc/nedmalloc.c | 5 +++--
>>   fast-import.c| 5 +++--
>>   revision.c   | 2 +-
>>   3 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
>> index 609ebba..a0a16eb 100644
>> --- a/compat/nedmalloc/nedmalloc.c
>> +++ b/compat/nedmalloc/nedmalloc.c
>> @@ -957,8 +957,9 @@ char *strdup(const char *s1)
>>   {
>>   char *s2 = 0;
>>   if (s1) {
>> -s2 = malloc(strlen(s1) + 1);
>> -strcpy(s2, s1);
>> +size_t len = strlen(s1) + 1;
>> +s2 = malloc(len);
>> +memcpy(s2, s1, len);
> 
> This leaves the last byte uninitialized; it was set to NUL by strcpy() before.

len is == strlen() +1, which should cover the NUL:

1 byte extra for NUL is allocated,
and memcpy will copy NUL from source.
(Or do I miss somethong ?)



--
To unsubscribe from this list: send the line "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 v1] Add Travis CI support

[Matthieu Moy]

Luke Diamand  writes:

> It would be less intrusive for the CI system to have a fork. Otherwise
> other people using git with the same CI system will get annoying merge
> conflicts,

What conflicts are you talking about? The ones in .travis.yml? The point
is to share this file so that people using the same system do not have
to change anything.

And, we're talking about a straightforward 28-lines long file, set up
essentially once and for all. Even if people ever modify it, I don't
forsee conflict resolution in such a simple file as a real problem.

> and we'll also end up with a repo littered with the control files from
> past CI systems if the CI system is ever changed.

Again, we're talking about a short and simple configuration file.

Sure, when we change something, we either get old files lying around or
have to remove the old files. But would we say "Git shouldn't have a
Makefile, because having a Makefile would mean we'd end up with a repo
littered with Makefiles the day we migrate to another build system"?

> From past experience, if it's configured to email people when things
> break, sooner or later it will email the wrong people, probably once
> every few seconds over a weekend.

Are you talking about your experience with Travis-CI in particular, or
with CI systems in general? Is the scenario where Travis-CI sends email
based on actual facts, or only speculation?

My experience with Travis-CI is that it just works (my experience is
limited, but I'm using it for git-multimail, and it's a really
convenient tool). It does send emails by default, but with a very
reasonable policy:

  http://docs.travis-ci.com/user/notifications/

  "By default, email notifications are sent to the committer and the
  commit author, if they are members of the repository (that is, they
  have push or admin permissions for public repositories, or if they
  have pull, push or admin permissions for private repositories)."

In short:

* If the tests always pass, nobody ever get any email from Travis-CI.

* When someone sends a pull-request that fails tests, that someone gets
  an automatic email about the failure. This saves one email round-trip
  "X sends a patch series, Junio notices the failure, Junio sends an
  email about the failure", and shortcuts this as "X sends a PR, and
  gets an email, possibly even before Junio notices".

> Automated testing is a Good Thing, but it's still software, so needs
> maintenance or it will break.

The point of using Travis-CI is precisely to use an externally
maintained system. It's not just software, it's a service (based on
software, obviously).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 60/68] prefer memcpy to strcpy

[René Scharfe]

Am 27.09.2015 um 15:13 schrieb René Scharfe:

Am 27.09.2015 um 15:06 schrieb Torsten Bögershausen:

On 2015-09-27 13.19, René Scharfe wrote:

Am 24.09.2015 um 23:08 schrieb Jeff King:

When we already know the length of a string (e.g., because
we just malloc'd to fit it), it's nicer to use memcpy than
strcpy, as it makes it more obvious that we are not going to
overflow the buffer (because the size we pass matches the
size in the allocation).

This also eliminates calls to strcpy, which make auditing
the code base harder.

Signed-off-by: Jeff King 
---
   compat/nedmalloc/nedmalloc.c | 5 +++--
   fast-import.c| 5 +++--
   revision.c   | 2 +-
   3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/compat/nedmalloc/nedmalloc.c
b/compat/nedmalloc/nedmalloc.c
index 609ebba..a0a16eb 100644
--- a/compat/nedmalloc/nedmalloc.c
+++ b/compat/nedmalloc/nedmalloc.c
@@ -957,8 +957,9 @@ char *strdup(const char *s1)
   {
   char *s2 = 0;
   if (s1) {
-s2 = malloc(strlen(s1) + 1);
-strcpy(s2, s1);
+size_t len = strlen(s1) + 1;
+s2 = malloc(len);
+memcpy(s2, s1, len);


This leaves the last byte uninitialized; it was set to NUL by
strcpy() before.


len is == strlen() +1, which should cover the NUL:

1 byte extra for NUL is allocated,
and memcpy will copy NUL from source.
(Or do I miss somethong ?)


No, you're right.  Sorry for the noise.

I fully blame this on lack of coffeine because my electric kettle just
broke. O_o


Thinking a bit more about it (slowly): The choice of the variable name 
might have been a factor as well.  When I see "len" for a string then I 
don't expect it to include the trailing NUL.  "size" would be better 
because I expect it to contain the number of bytes needed to store an 
object.


René

--
To unsubscribe from this list: send the line "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/2] t3203: test 'detached at' after checkout --detach

[Matthieu Moy]

This currently fails: the output is 'HEAD detached at HEAD'.
---
 t/t3203-branch-output.sh | 13 +
 1 file changed, 13 insertions(+)

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index f51d0f3..bf24dbf 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -106,6 +106,19 @@ EOF
test_i18ncmp expect actual
 '
 
+test_expect_failure 'git branch shows detached HEAD properly after checkout 
--detach' '
+   git checkout master &&
+   cat >expect expect 

[PATCH 2/2] status: don't say 'HEAD detached at HEAD'

[Matthieu Moy]

After using "git checkout --detach", the reflog is left with an entry
like

  checkout: moving from ... to HEAD

This message is parsed to generate the 'HEAD detached at' message in
'git branch' and 'git status', which leads to the not-so-useful message
'HEAD detached at HEAD'.

Instead, when parsing such reflog entry, resolve HEAD to the
corresponding commit in the reflog, so that the message becomes 'HEAD
detached at $sha1'.
---
Another possible fix is to avoid creating such reflog entry. But
anyway, this patch remains a good thing to do and it does fix the
issue.

I won't have time to work on fixing the reflog soon, but it may be a
nice microproject.

 t/t3203-branch-output.sh | 2 +-
 wt-status.c  | 6 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index bf24dbf..16efe7a 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -106,7 +106,7 @@ EOF
test_i18ncmp expect actual
 '
 
-test_expect_failure 'git branch shows detached HEAD properly after checkout 
--detach' '
+test_expect_success 'git branch shows detached HEAD properly after checkout 
--detach' '
git checkout master &&
cat >expect buf,
+ find_unique_abbrev(nsha1, DEFAULT_ABBREV));
+   return 1;
+   }
strbuf_add(>buf, target, end - target);
return 1;
 }
-- 
2.5.0.402.g8854c44

--
To unsubscribe from this list: send the line "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] Pass amend to pre-commit hook

[Øystein Walle]

Jeff King  peff.net> writes:

> 
> On Mon, Sep 14, 2015 at 01:14:20PM +0100, Alan Clucas wrote:
> 
> > Pass a single parameter 'amend' to the pre-commit hook when performing a
> > commit amend.
> 
> I think this is a sensible thing to want, and it has come up a few
> times. I'm not sure why the last round didn't get merged, though. Looks
> like it just slipped through the cracks.
> 
> Here are the relevant threads:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/260122
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/260245
> 
> Looks like there was some question of what to pass in the normal,
> non-amend case. I've added interested parties from the original thread
> to the cc here.
> 
> -Peff
> 

There were so many different ways of solving them that we weren't able
to decide between them:

Assuming we give the string "amend" as the hook's argv[1], what to do
when --amend is not used? We can pass nothing, or the empty string, or
the string "noamend". Then there was the suggestion of exporting
GIT_AMEND=1 or something like that. In any case, what to do when we want
to pass more arguments? Should we let the hook check argv[1] to decide
whether --amend was used, argv[2] to check whether {some scenario here}
is the case? Or make the hook author effectively implement options
parsing?

And then it died out...

In my totally unprofessional opinion anything more complex than maybe
passing "amend" in argv[1] is unwarranted. Since I posted my first patch
almost a year ago there has been no discussion regarding passing other
information along to pre-commit. Furthermore --amend is the only thing I
know of that a pre-commit hook cannot discover on its own. On the other
hand the desire for this has popped up at least twice on #git in the
same time span.

Alan Clucas' solution looks fine to me. It is essentially the same as
mine. But mine had tests and whatnot ;-)

Øsse

[PATCH] git-multimail version 1.2 RC1

[Matthieu Moy]

The changes are described in CHANGES.

Contributions-by: Job Snijders 
Contributions-by: Richard Hansen 
Contributions-by: Elijah Newren 
Contributions-by: Michael Haggerty 
Contributions-by: Paul Sokolovsky 
Contributions-by: Vadim Zeitlin 
Contributions-by: Edward d'Auvergne 
Contributions-by: Elijah Newren 
Signed-off-by: Matthieu Moy 
---

This should not go into Git 2.6, but I'll release git-multimail 1.2
final rather soon, so this patch can be queued and merged after 2.6 is
released.

 contrib/hooks/multimail/CHANGES |  49 ++
 contrib/hooks/multimail/CONTRIBUTING.rst|  30 +
 contrib/hooks/multimail/README  | 255 ---
 contrib/hooks/multimail/README.Git  |   4 +-
 contrib/hooks/multimail/doc/gerrit.rst  |  56 ++
 contrib/hooks/multimail/doc/gitolite.rst| 109 +++
 contrib/hooks/multimail/git_multimail.py| 932 
 contrib/hooks/multimail/migrate-mailhook-config |   2 +-
 contrib/hooks/multimail/post-receive.example|   8 +-
 9 files changed, 1221 insertions(+), 224 deletions(-)
 create mode 100644 contrib/hooks/multimail/CONTRIBUTING.rst
 create mode 100644 contrib/hooks/multimail/doc/gerrit.rst
 create mode 100644 contrib/hooks/multimail/doc/gitolite.rst

diff --git a/contrib/hooks/multimail/CHANGES b/contrib/hooks/multimail/CHANGES
index 6bb1230..1c41e04 100644
--- a/contrib/hooks/multimail/CHANGES
+++ b/contrib/hooks/multimail/CHANGES
@@ -1,3 +1,52 @@
+Release 1.2.0 RC1
+=
+
+This release has been well tested, but contains possibly controversial
+changes visible to the user. It is an alpha release in the sense that
+the user-interface may still change before the final release depending
+on the user feedback.
+
+* It is now possible to exclude some refs (e.g. exclude some branches
+  or tags). See refFilterDoSendRegex, refFilterDontSendRegex,
+  refFilterInclusionRegex and refFilterExclusionRegex.
+
+* New commitEmailFormat option which can be set to "html" to generate
+  simple colorized diffs using HTML for the commit emails.
+
+* git-multimail can now be ran as a Gerrit ref-updated hook, or from
+  Atlassian Stash.
+
+* The From: field is now more customizeable. It can be set
+  independently for refchange emails and commit emails (see
+  fromCommit, fromRefChange). The special values pusher and author can
+  be used in these configuration variable.
+
+* A new command-line option, --version, was added. The version is also
+  available in the X-Git-Multimail-Version header of sent emails.
+
+* Set X-Git-NotificationType header to differentiate the various types
+  of notifications. Current values are: diff, ref_changed_plus_diff,
+  ref_changed.
+
+* Preliminary support for Python 3. The testsuite passes with Python 3,
+  but it has not received as much testing as the Python 2 version yet.
+
+* Several encoding-related fixes. UTF-8 characters work in more
+  situations (but non-ascii characters in email address are still not
+  supported).
+
+* The testsuite and its documentation has been greatly improved.
+
+Plus all the bugfixes from version 1.1.1.
+
+This version has been tested with Python 2.4 and 2.6 to 3.5, and Git
+v1.7.10-406-gdc801e7, git-1.8.2.3 and 2.6.0-rc0. Git versions prior to
+v1.7.10-406-gdc801e7 probably work, but cannot run the testsuite
+properly.
+
+Changes since 1.2 Alpha 1: testsuite improvements and fixes, better
+HTML formatting.
+
 Release 1.1.1 (bugfix-only release)
 ===
 
diff --git a/contrib/hooks/multimail/CONTRIBUTING.rst 
b/contrib/hooks/multimail/CONTRIBUTING.rst
new file mode 100644
index 000..09efdb0
--- /dev/null
+++ b/contrib/hooks/multimail/CONTRIBUTING.rst
@@ -0,0 +1,30 @@
+git-multimail is an open-source project, built by volunteers. We would
+welcome your help!
+
+The current maintainers are Michael Haggerty 
+and Matthieu Moy .
+
+Please note that although a copy of git-multimail is distributed in
+the "contrib" section of the main Git project, development takes place
+in a separate git-multimail repository on GitHub:
+
+https://github.com/git-multimail/git-multimail
+
+Whenever enough changes to git-multimail have accumulated, a new
+code-drop of git-multimail will be submitted for inclusion in the Git
+project.
+
+We use the GitHub issue tracker to keep track of bugs and feature
+requests, and we use GitHub pull requests to exchange patches (though,
+if you prefer, you can send patches via the Git mailing list with CC
+to the maintainers). Please sign off your patches as per the `Git
+project practice
+`__.
+
+General discussion of git-multimail can take place on the main Git

Re: Single brackets matching in .gitignore rules

[Andreas Schwab]

Andrey Loskutov  writes:

> Next, the surprising table for ']':
> --
> rule  | file | match?
> --
> ]   ]  true
> ]* ]  true
> *] ]  true
> *] a]true
> --
>
> Here Git does not give up on parsing, treats unmatched ']' character 
> literally, and doesn't dislike that it is an "unmatched" end of a broken 
> character group.
> Why?

] by itself is not special in a glob.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "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 60/68] prefer memcpy to strcpy

[René Scharfe]

Am 24.09.2015 um 23:08 schrieb Jeff King:

When we already know the length of a string (e.g., because
we just malloc'd to fit it), it's nicer to use memcpy than
strcpy, as it makes it more obvious that we are not going to
overflow the buffer (because the size we pass matches the
size in the allocation).

This also eliminates calls to strcpy, which make auditing
the code base harder.

Signed-off-by: Jeff King 
---
  compat/nedmalloc/nedmalloc.c | 5 +++--
  fast-import.c| 5 +++--
  revision.c   | 2 +-
  3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
index 609ebba..a0a16eb 100644
--- a/compat/nedmalloc/nedmalloc.c
+++ b/compat/nedmalloc/nedmalloc.c
@@ -957,8 +957,9 @@ char *strdup(const char *s1)
  {
char *s2 = 0;
if (s1) {
-   s2 = malloc(strlen(s1) + 1);
-   strcpy(s2, s1);
+   size_t len = strlen(s1) + 1;
+   s2 = malloc(len);
+   memcpy(s2, s1, len);


This leaves the last byte uninitialized; it was set to NUL by strcpy() 
before.



}
return s2;
  }
diff --git a/fast-import.c b/fast-import.c
index 895c6b4..cf6d8bc 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -644,8 +644,9 @@ static void *pool_calloc(size_t count, size_t size)

  static char *pool_strdup(const char *s)
  {
-   char *r = pool_alloc(strlen(s) + 1);
-   strcpy(r, s);
+   size_t len = strlen(s) + 1;
+   char *r = pool_alloc(len);
+   memcpy(r, s, len);


Same here.


return r;
  }

diff --git a/revision.c b/revision.c
index af2a18e..2236463 100644
--- a/revision.c
+++ b/revision.c
@@ -38,7 +38,7 @@ char *path_name(const struct name_path *path, const char 
*name)
}
n = xmalloc(len);
m = n + len - (nlen + 1);
-   strcpy(m, name);
+   memcpy(m, name, nlen + 1);


This copies the NUL byte terminating the string, so it's OK.  However, I 
wonder if using a strbuf for building the path in one go instead would 
simplify this function without too much of a performance impact.



for (p = path; p; p = p->up) {
if (p->elem_len) {
m -= p->elem_len + 1;



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