Re: [PATCH 2/2] pretty: use fmt_output_email_subject()

[René Scharfe]

Am 01.03.2017 um 12:37 schrieb René Scharfe:

Add the email-style subject prefix (e.g. "Subject: [PATCH] ") directly
when it's needed instead of letting log_write_email_headers() prepare
it in a static buffer in advance.  This simplifies storage ownership and
code flow.

Signed-off-by: Rene Scharfe <l@web.de>
---
This slows down the last three tests in p4000 by ca. 3% for some reason,
so we may want to only do the first part for now, which is performance
neutral on my machine.


Update below.


diff --git a/commit.h b/commit.h
index 9c12abb911..459daef94a 100644
--- a/commit.h
+++ b/commit.h
@@ -142,21 +142,24 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
 }

+struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
+
 struct pretty_print_context {
/*
 * Callers should tweak these to change the behavior of pp_* functions.
 */
enum cmit_fmt fmt;
int abbrev;
-   const char *subject;
const char *after_subject;
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
+   unsigned print_email_subject:1;


Turning this into an int restores performance according to p4000. 
Didn't know that bitfields can be *that* expensive.


René

Re: What's cooking in git.git (Mar 2017, #01; Wed, 1)

[René Scharfe]

Am 01.03.2017 um 23:35 schrieb Junio C Hamano:
> * rs/log-email-subject (2017-03-01) 2 commits
>  - pretty: use fmt_output_email_subject()
>  - log-tree: factor out fmt_output_email_subject()
> 
>  Code clean-up.
> 
>  Will merge to 'next'.

Could you please squash this in?  We only use a single context (as
opposed to an array), so it doesn't have to be especially compact,
and using a bitfield slows down half of the tests in p4000 by 3%
for me.

---
 commit.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/commit.h b/commit.h
index 459daef94a..528272ac9b 100644
--- a/commit.h
+++ b/commit.h
@@ -154,7 +154,7 @@ struct pretty_print_context {
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
-   unsigned print_email_subject:1;
+   int print_email_subject;
int expand_tabs_in_log;
int need_8bit_cte;
char *notes_message;
-- 
2.12.0

[PATCH 1/2] log-tree: factor out fmt_output_email_subject()

[René Scharfe]

Use a strbuf to store the subject prefix string and move its
construction into its own function.  This gets rid of two arbitrary
length limits and allows the string to be added by callers directly.

Signed-off-by: Rene Scharfe 
---
 log-tree.c | 40 
 log-tree.h |  1 +
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 8c2415747a..44febb75ab 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -332,35 +332,33 @@ void fmt_output_commit(struct strbuf *filename,
strbuf_release();
 }
 
+void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
+{
+   if (opt->total > 0) {
+   strbuf_addf(sb, "Subject: [%s%s%0*d/%d] ",
+   opt->subject_prefix,
+   *opt->subject_prefix ? " " : "",
+   digits_in_number(opt->total),
+   opt->nr, opt->total);
+   } else if (opt->total == 0 && opt->subject_prefix && 
*opt->subject_prefix) {
+   strbuf_addf(sb, "Subject: [%s] ",
+   opt->subject_prefix);
+   } else {
+   strbuf_addstr(sb, "Subject: ");
+   }
+}
+
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 const char **subject_p,
 const char **extra_headers_p,
 int *need_8bit_cte_p)
 {
-   const char *subject = NULL;
+   static struct strbuf subject = STRBUF_INIT;
const char *extra_headers = opt->extra_headers;
const char *name = oid_to_hex(opt->zero_commit ?
  _oid : >object.oid);
 
*need_8bit_cte_p = 0; /* unknown */
-   if (opt->total > 0) {
-   static char buffer[64];
-   snprintf(buffer, sizeof(buffer),
-"Subject: [%s%s%0*d/%d] ",
-opt->subject_prefix,
-*opt->subject_prefix ? " " : "",
-digits_in_number(opt->total),
-opt->nr, opt->total);
-   subject = buffer;
-   } else if (opt->total == 0 && opt->subject_prefix && 
*opt->subject_prefix) {
-   static char buffer[256];
-   snprintf(buffer, sizeof(buffer),
-"Subject: [%s] ",
-opt->subject_prefix);
-   subject = buffer;
-   } else {
-   subject = "Subject: ";
-   }
 
fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
graph_show_oneline(opt->graph);
@@ -417,7 +415,9 @@ void log_write_email_headers(struct rev_info *opt, struct 
commit *commit,
opt->diffopt.stat_sep = buffer;
strbuf_release();
}
-   *subject_p = subject;
+   strbuf_reset();
+   fmt_output_email_subject(, opt);
+   *subject_p = subject.buf;
*extra_headers_p = extra_headers;
 }
 
diff --git a/log-tree.h b/log-tree.h
index c8116e60cd..dd75dd7770 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -30,5 +30,6 @@ void load_ref_decorations(int flags);
 #define FORMAT_PATCH_NAME_MAX 64
 void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info 
*);
+void fmt_output_email_subject(struct strbuf *, struct rev_info *);
 
 #endif
-- 
2.12.0

[PATCH 2/2] pretty: use fmt_output_email_subject()

[René Scharfe]

Add the email-style subject prefix (e.g. "Subject: [PATCH] ") directly
when it's needed instead of letting log_write_email_headers() prepare
it in a static buffer in advance.  This simplifies storage ownership and
code flow.

Signed-off-by: Rene Scharfe 
---
This slows down the last three tests in p4000 by ca. 3% for some reason,
so we may want to only do the first part for now, which is performance
neutral on my machine.

 builtin/log.c  | 5 +++--
 builtin/shortlog.c | 2 +-
 commit.h   | 6 --
 log-tree.c | 9 +++--
 log-tree.h | 1 -
 pretty.c   | 7 ---
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc2d8..281af8c1ec 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -989,8 +989,7 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", 
rev, quiet))
return;
 
-   log_write_email_headers(rev, head, , _subject,
-   _8bit_cte);
+   log_write_email_headers(rev, head, _subject, _8bit_cte);
 
for (i = 0; !need_8bit_cte && i < nr; i++) {
const char *buf = get_commit_buffer(list[i], NULL);
@@ -1005,6 +1004,8 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
msg = body;
pp.fmt = CMIT_FMT_EMAIL;
pp.date_mode.type = DATE_RFC2822;
+   pp.rev = rev;
+   pp.print_email_subject = 1;
pp_user_info(, NULL, , committer, encoding);
pp_title_line(, , , encoding, need_8bit_cte);
pp_remainder(, , , 0);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index c9585d475d..f78bb4818d 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -148,7 +148,7 @@ void shortlog_add_commit(struct shortlog *log, struct 
commit *commit)
 
ctx.fmt = CMIT_FMT_USERFORMAT;
ctx.abbrev = log->abbrev;
-   ctx.subject = "";
+   ctx.print_email_subject = 1;
ctx.after_subject = "";
ctx.date_mode.type = DATE_NORMAL;
ctx.output_encoding = get_log_output_encoding();
diff --git a/commit.h b/commit.h
index 9c12abb911..459daef94a 100644
--- a/commit.h
+++ b/commit.h
@@ -142,21 +142,24 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
 }
 
+struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
+
 struct pretty_print_context {
/*
 * Callers should tweak these to change the behavior of pp_* functions.
 */
enum cmit_fmt fmt;
int abbrev;
-   const char *subject;
const char *after_subject;
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
+   unsigned print_email_subject:1;
int expand_tabs_in_log;
int need_8bit_cte;
char *notes_message;
struct reflog_walk_info *reflog_info;
+   struct rev_info *rev;
const char *output_encoding;
struct string_list *mailmap;
int color;
@@ -175,7 +178,6 @@ struct userformat_want {
 };
 
 extern int has_non_ascii(const char *text);
-struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
 extern const char *logmsg_reencode(const struct commit *commit,
   char **commit_encoding,
   const char *output_encoding);
diff --git a/log-tree.c b/log-tree.c
index 44febb75ab..4618dd04ca 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -349,11 +349,9 @@ void fmt_output_email_subject(struct strbuf *sb, struct 
rev_info *opt)
 }
 
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
-const char **subject_p,
 const char **extra_headers_p,
 int *need_8bit_cte_p)
 {
-   static struct strbuf subject = STRBUF_INIT;
const char *extra_headers = opt->extra_headers;
const char *name = oid_to_hex(opt->zero_commit ?
  _oid : >object.oid);
@@ -415,9 +413,6 @@ void log_write_email_headers(struct rev_info *opt, struct 
commit *commit,
opt->diffopt.stat_sep = buffer;
strbuf_release();
}
-   strbuf_reset();
-   fmt_output_email_subject(, opt);
-   *subject_p = subject.buf;
*extra_headers_p = extra_headers;
 }
 
@@ -602,8 +597,10 @@ void show_log(struct rev_info *opt)
 */
 
if (cmit_fmt_is_mail(opt->commit_format)) {
-   log_write_email_headers(opt, commit, , 
_headers,
+   log_write_email_headers(opt, commit, _headers,
_8bit_cte);
+   ctx.rev = opt;
+   ctx.print_email_subject = 1;
} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
fputs(diff_get_color_opt(>diffopt, 

Re: git status --> Out of memory, realloc failed

[René Scharfe]

Am 25.02.2017 um 11:13 schrieb Carsten Fuchs:

Dear Git group,

I use Git at a web hosting service, where my user account has a memory
limit of 768 MB:

(uiserver):p7715773:~$ uname -a
Linux infongp-de15 3.14.0-ui16322-uiabi1-infong-amd64 #1 SMP Debian
3.14.79-2~ui80+4 (2016-11-17) x86_64 GNU/Linux


What's the output of "ulimit -a"?


(uiserver):p7715773:~$ git --version
git version 2.1.4


That's quite old.  Can you try a more recent version easily (2.12.0 just 
came out)?  I don't remember a specific fix, and memory usage perhaps 
even increased with newer versions, but ruling out already fixed issues 
would be nice.



(uiserver):p7715773:~/cafu$ git gc
Zähle Objekte: 44293, Fertig.
Komprimiere Objekte: 100% (24534/24534), Fertig.
Schreibe Objekte: 100% (44293/44293), Fertig.
Total 44293 (delta 17560), reused 41828 (delta 16708)

(uiserver):p7715773:~/cafu$ git status
fatal: Out of memory, realloc failed
fatal: recursion detected in die handler

The repository is tracking about 19000 files which together take 260 MB.
The git server version is 2.7.4.1.g5468f9e (Bitbucket)


Is your repository publicly accessible?

René

Re: [PATCH 0/6] Use time_t

[René Scharfe]

Am 28.02.2017 um 21:54 schrieb Johannes Schindelin:

Hi Junio,

On Tue, 28 Feb 2017, Junio C Hamano wrote:


René Scharfe <l@web.de> writes:


Am 28.02.2017 um 15:28 schrieb Jeff King:


It looks from the discussion like the sanest path forward is our own
signed-64bit timestamp_t. That's unfortunate compared to using the
standard time_t, but hopefully it would reduce the number of knobs
(like TIME_T_IS_INT64) in the long run.


Glibc will get a way to enable 64-bit time_t on 32-bit platforms
eventually (https://sourceware.org/glibc/wiki/Y2038ProofnessDesign).
Can platforms that won't provide a 64-bit time_t by 2038 be actually
used at that point?  How would we get time information on them?  How
would a custom timestamp_t help us?


That's a sensible "wait, let's step back a bit".  I take it that you are
saying "time_t is just fine", and I am inclined to agree.

Right now, they may be able to have future timestamps ranging to
year 2100 and switching to time_t would limit their ability to
express future time to 2038 but they would be able to express
timestamp in the past to cover most of 20th century.  Given that
these 32-bit time_t software platforms will die off before year 2038
(either by underlying hardware getting obsolete, or software updated
to handle 64-bit time_t), the (temporary) loss of 2038-2100 range
would not be too big a deal to warrant additional complexity.


You seem to assume that time_t is required to be signed. But from my
understanding that is only guaranteed by POSIX, not by ISO C.

We may very well buy ourselves a ton of trouble if we decide to switch to
`time_t` rather than to `int64_t`.


True, and time_t doesn't even have to be an integer type.  But which 
platforms capable of running git use something else than int32_t or int64_t?


René

Re: [PATCH 0/6] Use time_t

[René Scharfe]

Am 01.03.2017 um 00:10 schrieb Johannes Schindelin:

Hi René,

On Tue, 28 Feb 2017, René Scharfe wrote:


Am 28.02.2017 um 21:54 schrieb Johannes Schindelin:


On Tue, 28 Feb 2017, Junio C Hamano wrote:


René Scharfe <l@web.de> writes:


Am 28.02.2017 um 15:28 schrieb Jeff King:


It looks from the discussion like the sanest path forward is our
own signed-64bit timestamp_t. That's unfortunate compared to
using the standard time_t, but hopefully it would reduce the
number of knobs (like TIME_T_IS_INT64) in the long run.


Glibc will get a way to enable 64-bit time_t on 32-bit platforms
eventually
(https://sourceware.org/glibc/wiki/Y2038ProofnessDesign).  Can
platforms that won't provide a 64-bit time_t by 2038 be actually
used at that point?  How would we get time information on them?
How would a custom timestamp_t help us?


That's a sensible "wait, let's step back a bit".  I take it that you
are saying "time_t is just fine", and I am inclined to agree.

Right now, they may be able to have future timestamps ranging to
year 2100 and switching to time_t would limit their ability to
express future time to 2038 but they would be able to express
timestamp in the past to cover most of 20th century.  Given that
these 32-bit time_t software platforms will die off before year 2038
(either by underlying hardware getting obsolete, or software updated
to handle 64-bit time_t), the (temporary) loss of 2038-2100 range
would not be too big a deal to warrant additional complexity.


You seem to assume that time_t is required to be signed. But from my
understanding that is only guaranteed by POSIX, not by ISO C.

We may very well buy ourselves a ton of trouble if we decide to switch
to `time_t` rather than to `int64_t`.


True, and time_t doesn't even have to be an integer type.  But which
platforms capable of running git use something else than int32_t or
int64_t?


That kind of thinking is dangerous. We don't know what platforms are
running Git, and we have a very clear example how we got it very wrong
recently, when we broke building with musl by requiring REG_STARTEND
support [*1*].


In general that's true, and if nobody can add to the list (glibc: 
int32_t on 32-bit platforms, int64_t on 64-bit platforms for now; 
NetBSD, OpenBSD, Windows int64_t) then we shouldn't make assumptions 
about time_t that go beyond the C standard -- at least not without 
verifying them, e.g. with asserts or tests.  I'd especially be 
interested in hearing about platforms that use a floating point type.


Systems lacking REG_STARTEND can use compat/regex/ -- that's doesn't 
sound too bad.  (I didn't follow the original discussion too closely, 
and don't mean to open it up again.)



So why gamble? If we switch to uint64_t, it would definitely provide the
smoothest upgrade path. It is what the code assumed implicitly when we
broke 32-bit in v2.9.1.


We can assume that time_t exists everywhere and is used by functions 
like gettimeofday(2) and localtime(3).  Why invent our own layer on top? 
 What would it be able to do that time_t alone can't?  I understand a 
need to handle times before 1970 for historical documents, but what good 
would it do to have the ability to handle dates beyond 2038, today?


Platforms that don't use the next two decades to provide a time_t that 
can store the current time by then will have bigger problems than a 
crippled git.



If anybody really wants to support negative timestamps, it should be done
on top of my work. My current patch series does not even start to try to
address the ramifications of negative timestamps (see e.g. the use of
strtoull() for parsing). It is quite unreasonable to ask for such a
fundamental design change when it could very easily be done incrementally
instead, when needed, by someone who needs it.


I'm confused.  Your patch series converts to time_t.  Why not use it? 
That would prepare ourselves for the bright future when we'll have a 
64-bit time_t in every libc. :)


René

Re: [PATCH 0/6] Use time_t

[René Scharfe]

Am 28.02.2017 um 15:28 schrieb Jeff King:

On Mon, Feb 27, 2017 at 10:30:20PM +0100, Johannes Schindelin wrote:


One notable fallout of this patch series is that on 64-bit Linux (and
other platforms where `unsigned long` is 64-bit), we now limit the range
of dates to LONG_MAX (i.e. the *signed* maximum value). This needs to be
done as `time_t` can be signed (and indeed is at least on my Ubuntu
setup).

Obviously, I think that we can live with that, and I hope that all
interested parties agree.


I do not just agree, but I think the move to a signed timestamp is a big
improvement. Git's object format is happy to represent times before
1970, but the code is not. I know this has been a pain for people who
import ancient histories into Git.

It looks from the discussion like the sanest path forward is our own
signed-64bit timestamp_t. That's unfortunate compared to using the
standard time_t, but hopefully it would reduce the number of knobs (like
TIME_T_IS_INT64) in the long run.


Glibc will get a way to enable 64-bit time_t on 32-bit platforms 
eventually (https://sourceware.org/glibc/wiki/Y2038ProofnessDesign). 
Can platforms that won't provide a 64-bit time_t by 2038 be actually 
used at that point?  How would we get time information on them?  How 
would a custom timestamp_t help us?


Regarding the need for knobs: We could let the compiler chose between 
strtoll() and strtol() based on the size of time_t, in an inline 
function.  The maximum value can be calculated using its size as well. 
And we could use PRIdMAX and cast to intmax_t for printing.


René

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 27.02.2017 um 23:33 schrieb Junio C Hamano:
> René Scharfe <l@web.de> writes:
> 
>> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
>>> René Scharfe <l@web.de> writes:
>>>
>>>>> diff --git a/apply.c b/apply.c
>>>>> index cbf7cc7f2..9219d2737 100644
>>>>> --- a/apply.c
>>>>> +++ b/apply.c
>>>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>>>>   if (!old_name)
>>>>>   return 0;
>>>>>
>>>>> - assert(patch->is_new <= 0);
>>>>
>>>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
>>>> line. Its intent was to handle diffs that contain an old name even for
>>>> a file that's created.  Citing from its commit message: "When we
>>>> cannot be sure by parsing the patch that it is not a creation patch,
>>>> we shouldn't complain when if there is no such a file."  Why not stop
>>>> complaining also in case we happen to know for sure that it's a
>>>> creation patch? I.e., why not replace the assert() with:
>>>>
>>>>if (patch->is_new == 1)
>>>>goto is_new;
>>>>
>>>>>   previous = previous_patch(state, patch, );
>>>
>>> When the caller does know is_new is true, old_name must be made/left
>>> NULL.  That is the invariant this assert is checking to catch an
>>> error in the calling code.
>>
>> There are some places in apply.c that set ->is_new to 1, but none of
>> them set ->old_name to NULL at the same time.
> 
> I thought all of these are flipping ->is_new that used to be -1
> (unknown) to (now we know it is new), and sets only new_name without
> doing anything to old_name, because they know originally both names
> are set to NULL.
> 
>> Having to keep these two members in sync sounds iffy anyway.  Perhaps
>> accessors can help, e.g. a setter which frees old_name when is_new is
>> set to 1, or a getter which returns NULL for old_name if is_new is 1.
> 
> Definitely, the setter would make it harder to make the mistake.

When I added setters, apply started to passed NULL to unlink(2) and
rmdir(2) in some of the new tests, which still failed.

That's because three of the diffs trigger both gitdiff_delete(), which
sets is_delete and old_name, and gitdiff_newfile(), which sets is_new
and new_name.  Create and delete equals move, right?  Or should we
error out at this point already?

The last new diff adds a new file that is copied.  Sounds impossible.
How about something like this, which forbids combinations that make no
sense.  Hope it's not too strict; at least all tests succeed.

---
 apply.c | 79 ++---
 1 file changed, 61 insertions(+), 18 deletions(-)

diff --git a/apply.c b/apply.c
index 21b0bebec5..6cb6860511 100644
--- a/apply.c
+++ b/apply.c
@@ -197,6 +197,14 @@ struct fragment {
 #define BINARY_DELTA_DEFLATED  1
 #define BINARY_LITERAL_DEFLATED 2
 
+enum patch_type {
+   CHANGE,
+   CREATE,
+   DELETE,
+   RENAME,
+   COPY
+};
+
 /*
  * This represents a "patch" to a file, both metainfo changes
  * such as creation/deletion, filemode and content changes represented
@@ -205,6 +213,7 @@ struct fragment {
 struct patch {
char *new_name, *old_name, *def_name;
unsigned int old_mode, new_mode;
+   enum patch_type type;
int is_new, is_delete;  /* -1 = unknown, 0 = false, 1 = true */
int rejected;
unsigned ws_rule;
@@ -229,6 +238,36 @@ struct patch {
struct object_id threeway_stage[3];
 };
 
+static int set_patch_type(struct patch *patch, enum patch_type type)
+{
+   if (patch->type != CHANGE && patch->type != type)
+   return error(_("conflicting patch types"));
+   patch->type = type;
+   switch (type) {
+   case CHANGE:
+   break;
+   case CREATE:
+   patch->is_new = 1;
+   patch->is_delete = 0;
+   free(patch->old_name);
+   patch->old_name = NULL;
+   break;
+   case DELETE:
+   patch->is_new = 0;
+   patch->is_delete = 1;
+   free(patch->new_name);
+   patch->new_name = NULL;
+   break;
+   case RENAME:
+   patch->is_rename = 1;
+   break;
+   case COPY:
+   patch->is_copy = 1;
+   break;
+   }
+   return 0;
+}
+
 static void free_fragment_list(struct fragment *list)
 {
while (list) {
@@ -907,13 +946,13 @@ static int parse_traditional_patch(struct ap

Re: [PATCH] strbuf: add strbuf_add_real_path()

[René Scharfe]

Am 27.02.2017 um 19:22 schrieb Brandon Williams:

On 02/25, René Scharfe wrote:

+void strbuf_add_real_path(struct strbuf *sb, const char *path)
+{
+   if (sb->len) {
+   struct strbuf resolved = STRBUF_INIT;
+   strbuf_realpath(, path, 1);
+   strbuf_addbuf(sb, );
+   strbuf_release();
+   } else
+   strbuf_realpath(sb, path, 1);


I know its not required but I would have braces on the 'else' branch
since they were needed on the 'if' branch.  But that's up to you and
your style :)


Personally I'd actually prefer them as well, but the project's style has 
traditionally been to avoid braces on such trailing single-line branches 
to save lines.  The CodingGuidelines for this topic have been clarified 
recently, though, and seem to require them now.  Interesting.


René

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

[René Scharfe]

Am 27.02.2017 um 23:27 schrieb Jakub Narębski:

W dniu 25.02.2017 o 20:27, René Scharfe pisze:

Both standard_header_field() and excluded_header_field() check if
there's a space after the buffer that's handed to them.  We already
check in the caller if that space is present.  Don't bother calling
the functions if it's missing, as they are guaranteed to return 0 in
that case, and remove the now redundant checks from them.

Signed-off-by: Rene Scharfe <l@web.de>
---
 commit.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/commit.c b/commit.c
index 173c6d3818..fab8269731 100644
--- a/commit.c
+++ b/commit.c
@@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct 
commit *commit, void *data)

 static inline int standard_header_field(const char *field, size_t len)
 {
-   return ((len == 4 && !memcmp(field, "tree ", 5)) ||
-   (len == 6 && !memcmp(field, "parent ", 7)) ||
-   (len == 6 && !memcmp(field, "author ", 7)) ||
-   (len == 9 && !memcmp(field, "committer ", 10)) ||
-   (len == 8 && !memcmp(field, "encoding ", 9)));
+   return ((len == 4 && !memcmp(field, "tree", 4)) ||
+   (len == 6 && !memcmp(field, "parent", 6)) ||
+   (len == 6 && !memcmp(field, "author", 6)) ||
+   (len == 9 && !memcmp(field, "committer", 9)) ||
+   (len == 8 && !memcmp(field, "encoding", 8)));


I agree (for what it is worth from me) with the rest of changes,
but I think current code is better self-documenting for this
function.


Having a function that is given a buffer/length pair and accessing the 
byte after it raises questions, though. :)


Nicer than keeping the space would be to use excluded_header_field() for 
standard headers as well as a next step, I think -- but that would be a 
bit slower.





 }

 static int excluded_header_field(const char *field, size_t len, const char 
**exclude)
@@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, 
size_t len, const char **exc

while (*exclude) {
size_t xlen = strlen(*exclude);
-   if (len == xlen &&
-   !memcmp(field, *exclude, xlen) && field[xlen] == ' ')
+   if (len == xlen && !memcmp(field, *exclude, xlen))
return 1;
exclude++;
}
@@ -1357,9 +1356,8 @@ static struct commit_extra_header 
*read_commit_extra_header_lines(
eof = memchr(line, ' ', next - line);
if (!eof)
eof = next;
-
-   if (standard_header_field(line, eof - line) ||
-   excluded_header_field(line, eof - line, exclude))
+   else if (standard_header_field(line, eof - line) ||
+excluded_header_field(line, eof - line, exclude))
continue;

it = xcalloc(1, sizeof(*it));

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 27.02.2017 um 21:04 schrieb Junio C Hamano:

René Scharfe <l@web.de> writes:


diff --git a/apply.c b/apply.c
index cbf7cc7f2..9219d2737 100644
--- a/apply.c
+++ b/apply.c
@@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
if (!old_name)
return 0;

-   assert(patch->is_new <= 0);


5c47f4c6 (builtin-apply: accept patch to an empty file) added that
line. Its intent was to handle diffs that contain an old name even for
a file that's created.  Citing from its commit message: "When we
cannot be sure by parsing the patch that it is not a creation patch,
we shouldn't complain when if there is no such a file."  Why not stop
complaining also in case we happen to know for sure that it's a
creation patch? I.e., why not replace the assert() with:

if (patch->is_new == 1)
goto is_new;


previous = previous_patch(state, patch, );


When the caller does know is_new is true, old_name must be made/left
NULL.  That is the invariant this assert is checking to catch an
error in the calling code.


There are some places in apply.c that set ->is_new to 1, but none of 
them set ->old_name to NULL at the same time.


Having to keep these two members in sync sounds iffy anyway.  Perhaps 
accessors can help, e.g. a setter which frees old_name when is_new is 
set to 1, or a getter which returns NULL for old_name if is_new is 1.


René

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[René Scharfe]

Am 27.02.2017 um 21:10 schrieb Junio C Hamano:

René Scharfe <l@web.de> writes:


Would it make sense to mirror the previously existing condition and
check for is_new instead?  I.e.:

if ((!patch->is_delete && !patch->new_name) ||
(!patch->is_new&& !patch->old_name)) {



Yes, probably.


or

if (!(patch->is_delete || patch->new_name) ||
!(patch->is_new|| patch->old_name)) {


This happens after calling parse_git_header() so we should know the
actual value of is_delete and is_new by now (instead of mistaking
-1 aka "unknown" as true), so this rewrite would also be OK.


The two variants are logically equivalent -- (!a && !b) == !(a || b).  I 
wonder if the second one may be harder to read, though.


René

Re: SHA1 collisions found

[René Scharfe]

Am 25.02.2017 um 20:04 schrieb brian m. carlson:
>>> So I think that the current scope left is best estimated by the
>>> following command:
>>>
>>>   git grep -P 'unsigned char\s+(\*|.*20)' | grep -v '^Documentation'
>>>
>>> So there are approximately 1200 call sites left, which is quite a bit of
>>> work.  I estimate between the work I've done and other people's
>>> refactoring work (such as the refs backend refactor), we're about 40%
>>> done.
> 
> As a note, I've been working on this pretty much nonstop since the
> collision announcement was made.  After another 27 commits, I've got it
> down from 1244 to 1119.
> 
> I plan to send another series out sometime after the existing series has
> hit next.  People who are interested can follow the object-id-part*
> branches at https://github.com/bk2204/git.

Perhaps the following script can help a bit; it converts local and static
variables in specified files.  It's just a simplistic parser which can get
at least shadowing variables, strings and comments wrong, so its results
need to be reviewed carefully.

I failed to come up with an equivalent Coccinelle patch so far. :-/

René


#!/bin/sh
while test $# -gt 0
do
file="$1"
tmp="$file.new"
test -f "$file" &&
perl -e '
use strict;
my %indent;
my %old;
my %new;
my $in_struct = 0;
while (<>) {
if (/^(\s*)}/) {
my $len = length $1;
foreach my $key (keys %indent) {
if ($len < length($indent{$key})) {
delete $indent{$key};
delete $old{$key};
delete $new{$key};
}
}
$in_struct = 0;
}
if (!$in_struct and /^(\s*)(static )?unsigned char 
(\w+)\[20\];$/) {
my $prefix = "$1$2";
my $name = $3;
$indent{$.} = $1;
$old{$.} = qr/(?)(?"$tmp" &&
mv "$tmp" "$file" ||
exit 1
shift
done

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

[René Scharfe]

Am 25.02.2017 um 21:15 schrieb Jeff King:

On Sat, Feb 25, 2017 at 08:27:40PM +0100, René Scharfe wrote:


Both standard_header_field() and excluded_header_field() check if
there's a space after the buffer that's handed to them.  We already
check in the caller if that space is present.  Don't bother calling
the functions if it's missing, as they are guaranteed to return 0 in
that case, and remove the now redundant checks from them.


Makes sense, and I couldn't spot any errors in your logic or in the
code.


Thanks for checking!


 static inline int standard_header_field(const char *field, size_t len)
 {
-   return ((len == 4 && !memcmp(field, "tree ", 5)) ||
-   (len == 6 && !memcmp(field, "parent ", 7)) ||
-   (len == 6 && !memcmp(field, "author ", 7)) ||
-   (len == 9 && !memcmp(field, "committer ", 10)) ||
-   (len == 8 && !memcmp(field, "encoding ", 9)));
+   return ((len == 4 && !memcmp(field, "tree", 4)) ||
+   (len == 6 && !memcmp(field, "parent", 6)) ||
+   (len == 6 && !memcmp(field, "author", 6)) ||
+   (len == 9 && !memcmp(field, "committer", 9)) ||
+   (len == 8 && !memcmp(field, "encoding", 8)));


Unrelated, but this could probably be spelled with a macro and strlen()
to avoid the magic numbers. It would probably be measurably slower for a
compiler which doesn't pre-compute strlen() on a string literal, though.


sizeof(string_constant) - 1 might be a better choice here than strlen().

René

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 25.02.2017 um 11:13 schrieb Vegard Nossum:

For the patches in the added testcases, we were crashing with:

git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' 
failed.

As it turns out, check_preimage() is prepared to handle these conditions,
so we can remove the assertion.

Found using AFL.

Signed-off-by: Vegard Nossum 

---

(I'm fully aware of how it looks to just delete an assertion to "fix" a
bug without any other changes to accomodate the condition that was
being tested for. I am definitely not an expert on this code, but as far
as I can tell -- both by reviewing and testing the code -- the function
really is prepared to handle the case where patch->is_new == 1, as it
will always hit another error condition if that is true. I've tried to
add more test cases to show what errors you can expect to see instead of
the assertion failure when trying to apply these nonsensical patches. If
you don't want to remove the assertion for whatever reason, please feel
free to take the testcases and add "# TODO: known breakage" or whatever.)
---
 apply.c |  1 -
 t/t4154-apply-git-header.sh | 36 
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index cbf7cc7f2..9219d2737 100644
--- a/apply.c
+++ b/apply.c
@@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
if (!old_name)
return 0;

-   assert(patch->is_new <= 0);


5c47f4c6 (builtin-apply: accept patch to an empty file) added that line. 
 Its intent was to handle diffs that contain an old name even for a 
file that's created.  Citing from its commit message: "When we cannot be 
sure by parsing the patch that it is not a creation patch, we shouldn't 
complain when if there is no such a file."  Why not stop complaining 
also in case we happen to know for sure that it's a creation patch? 
I.e., why not replace the assert() with:


if (patch->is_new == 1)
goto is_new;


previous = previous_patch(state, patch, );

if (status)
diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
index d651af4a2..c440c48ad 100755
--- a/t/t4154-apply-git-header.sh
+++ b/t/t4154-apply-git-header.sh
@@ -12,4 +12,40 @@ rename new 0
 EOF
 '

+test_expect_success 'apply deleted file mode / new file mode / wrong mode' '
+   test_must_fail git apply << EOF
+diff --git a/. b/.
+deleted file mode
+new file mode
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / wrong type' '
+   mkdir x &&
+   chmod 755 x &&
+   test_must_fail git apply << EOF
+diff --git a/x b/x
+deleted file mode 160755
+new file mode
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / already exists' 
'
+   touch 1 &&
+   chmod 644 1 &&
+   test_must_fail git apply << EOF
+diff --git a/1 b/1
+deleted file mode 100644
+new file mode
+EOF
+'
+
+test_expect_success 'apply new file mode / copy from / nonexistant file' '
+   test_must_fail git apply << EOF
+diff --git a/. b/.
+new file mode
+copy from
+EOF
+'
+
 test_done

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[René Scharfe]

Am 25.02.2017 um 11:13 schrieb Vegard Nossum:

If we have a patch like the one in the new test-case, then we will
try to rename a non-existant empty file, i.e. patch->old_name will
be NULL. In this case, a NULL entry will be added to fn_table, which
is not allowed (a subsequent binary search will die with a NULL
pointer dereference).

The patch file is completely bogus as it tries to rename something
that is known not to exist, so we can throw an error for this.

Found using AFL.

Signed-off-by: Vegard Nossum 
---
 apply.c |  3 ++-
 t/t4154-apply-git-header.sh | 15 +++
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100755 t/t4154-apply-git-header.sh

diff --git a/apply.c b/apply.c
index 0e2caeab9..cbf7cc7f2 100644
--- a/apply.c
+++ b/apply.c
@@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
patch->old_name = xstrdup(patch->def_name);
patch->new_name = xstrdup(patch->def_name);
}
-   if (!patch->is_delete && !patch->new_name) {
+   if ((!patch->is_delete && !patch->new_name) ||
+   (patch->is_rename && !patch->old_name)) {


Would it make sense to mirror the previously existing condition and 
check for is_new instead?  I.e.:


if ((!patch->is_delete && !patch->new_name) ||
(!patch->is_new&& !patch->old_name)) {

or

if (!(patch->is_delete || patch->new_name) ||
!(patch->is_new|| patch->old_name)) {

René

[PATCH 2/2] commit: don't check for space twice when looking for header

[René Scharfe]

Both standard_header_field() and excluded_header_field() check if
there's a space after the buffer that's handed to them.  We already
check in the caller if that space is present.  Don't bother calling
the functions if it's missing, as they are guaranteed to return 0 in
that case, and remove the now redundant checks from them.

Signed-off-by: Rene Scharfe 
---
 commit.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/commit.c b/commit.c
index 173c6d3818..fab8269731 100644
--- a/commit.c
+++ b/commit.c
@@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct 
commit *commit, void *data)
 
 static inline int standard_header_field(const char *field, size_t len)
 {
-   return ((len == 4 && !memcmp(field, "tree ", 5)) ||
-   (len == 6 && !memcmp(field, "parent ", 7)) ||
-   (len == 6 && !memcmp(field, "author ", 7)) ||
-   (len == 9 && !memcmp(field, "committer ", 10)) ||
-   (len == 8 && !memcmp(field, "encoding ", 9)));
+   return ((len == 4 && !memcmp(field, "tree", 4)) ||
+   (len == 6 && !memcmp(field, "parent", 6)) ||
+   (len == 6 && !memcmp(field, "author", 6)) ||
+   (len == 9 && !memcmp(field, "committer", 9)) ||
+   (len == 8 && !memcmp(field, "encoding", 8)));
 }
 
 static int excluded_header_field(const char *field, size_t len, const char 
**exclude)
@@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, 
size_t len, const char **exc
 
while (*exclude) {
size_t xlen = strlen(*exclude);
-   if (len == xlen &&
-   !memcmp(field, *exclude, xlen) && field[xlen] == ' ')
+   if (len == xlen && !memcmp(field, *exclude, xlen))
return 1;
exclude++;
}
@@ -1357,9 +1356,8 @@ static struct commit_extra_header 
*read_commit_extra_header_lines(
eof = memchr(line, ' ', next - line);
if (!eof)
eof = next;
-
-   if (standard_header_field(line, eof - line) ||
-   excluded_header_field(line, eof - line, exclude))
+   else if (standard_header_field(line, eof - line) ||
+excluded_header_field(line, eof - line, exclude))
continue;
 
it = xcalloc(1, sizeof(*it));
-- 
2.12.0

[PATCH 1/2] commit: be more precise when searching for headers

[René Scharfe]

Search for a space character only within the current line in
read_commit_extra_header_lines() instead of searching in the whole
buffer (and possibly beyond, if it's not NUL-terminated) and then
discarding any results after the end of the current line.

Signed-off-by: Rene Scharfe 
---
 commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 2cf85158b4..173c6d3818 100644
--- a/commit.c
+++ b/commit.c
@@ -1354,8 +1354,8 @@ static struct commit_extra_header 
*read_commit_extra_header_lines(
strbuf_reset();
it = NULL;
 
-   eof = strchr(line, ' ');
-   if (next <= eof)
+   eof = memchr(line, ' ', next - line);
+   if (!eof)
eof = next;
 
if (standard_header_field(line, eof - line) ||
-- 
2.12.0

[PATCH] strbuf: add strbuf_add_real_path()

[René Scharfe]

Add a function for appending the canonized absolute pathname of a given
path to a strbuf.  It keeps the existing contents intact, as expected of
a function of the strbuf_add() family, while avoiding copying the result
if the given strbuf is empty.  It's more consistent with the rest of the
strbuf API than strbuf_realpath(), which it's wrapping.

Also add a semantic patch demonstrating its intended usage and apply it
to the current tree.  Using strbuf_add_real_path() instead of calling
strbuf_addstr() and real_path() avoids an extra copy to a static buffer.

Signed-off-by: Rene Scharfe 
---
 contrib/coccinelle/strbuf.cocci |  6 ++
 setup.c |  2 +-
 strbuf.c| 11 +++
 strbuf.h| 14 ++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 63995f22ff..1d580e49b0 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -38,3 +38,9 @@ expression E1, E2, E3;
 @@
 - strbuf_addstr(E1, find_unique_abbrev(E2, E3));
 + strbuf_add_unique_abbrev(E1, E2, E3);
+
+@@
+expression E1, E2;
+@@
+- strbuf_addstr(E1, real_path(E2));
++ strbuf_add_real_path(E1, E2);
diff --git a/setup.c b/setup.c
index 967f289f1e..f14cbcd338 100644
--- a/setup.c
+++ b/setup.c
@@ -254,7 +254,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char 
*gitdir)
if (!is_absolute_path(data.buf))
strbuf_addf(, "%s/", gitdir);
strbuf_addbuf(, );
-   strbuf_addstr(sb, real_path(path.buf));
+   strbuf_add_real_path(sb, path.buf);
ret = 1;
} else {
strbuf_addstr(sb, gitdir);
diff --git a/strbuf.c b/strbuf.cq
index 8fec6579f7..ace58e7367 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -707,6 +707,17 @@ void strbuf_add_absolute_path(struct strbuf *sb, const 
char *path)
strbuf_addstr(sb, path);
 }
 
+void strbuf_add_real_path(struct strbuf *sb, const char *path)
+{
+   if (sb->len) {
+   struct strbuf resolved = STRBUF_INIT;
+   strbuf_realpath(, path, 1);
+   strbuf_addbuf(sb, );
+   strbuf_release();
+   } else
+   strbuf_realpath(sb, path, 1);
+}
+
 int printf_ln(const char *fmt, ...)
 {
int ret;
diff --git a/strbuf.h b/strbuf.h
index cf1b5409e7..cf8e4bf532 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -441,6 +441,20 @@ extern int strbuf_getcwd(struct strbuf *sb);
  */
 extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
 
+/**
+ * Canonize `path` (make it absolute, resolve symlinks, remove extra
+ * slashes) and append it to `sb`.  Die with an informative error
+ * message if there is a problem.
+ *
+ * The directory part of `path` (i.e., everything up to the last
+ * dir_sep) must denote a valid, existing directory, but the last
+ * component need not exist.
+ *
+ * Callers that don't mind links should use the more lightweight
+ * strbuf_add_absolute_path() instead.
+ */
+extern void strbuf_add_real_path(struct strbuf *sb, const char *path);
+
 
 /**
  * Normalize in-place the path contained in the strbuf. See
-- 
2.12.0

[PATCH] cocci: use ALLOC_ARRAY

[René Scharfe]

Add a semantic patch for using ALLOC_ARRAY to allocate arrays and apply
the transformation on the current source tree.  The macro checks for
multiplication overflow and infers the element size automatically; the
result is shorter and safer code.

Signed-off-by: Rene Scharfe 
---
 contrib/coccinelle/array.cocci | 16 
 worktree.c |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index 2d7f25d99f..4ba98b7eaf 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci
@@ -24,3 +24,19 @@ expression n;
 @@
 - memcpy(dst, src, n * sizeof(T));
 + COPY_ARRAY(dst, src, n);
+
+@@
+type T;
+T *ptr;
+expression n;
+@@
+- ptr = xmalloc(n * sizeof(*ptr));
++ ALLOC_ARRAY(ptr, n);
+
+@@
+type T;
+T *ptr;
+expression n;
+@@
+- ptr = xmalloc(n * sizeof(T));
++ ALLOC_ARRAY(ptr, n);
diff --git a/worktree.c b/worktree.c
index d633761575..d7b911aac7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -175,7 +175,7 @@ struct worktree **get_worktrees(unsigned flags)
struct dirent *d;
int counter = 0, alloc = 2;
 
-   list = xmalloc(alloc * sizeof(struct worktree *));
+   ALLOC_ARRAY(list, alloc);
 
list[counter++] = get_main_worktree();
 
-- 
2.12.0

[PATCH] sha1_file: release fallback base's memory in unpack_entry()

[René Scharfe]

If a pack entry that's used as a delta base is corrupt, unpack_entry()
marks it as unusable and then searches the object again in the hope that
it can be found in another pack or in a loose file.  The memory for this
external base object is never released.  Free it after use.

Signed-off-by: Rene Scharfe 
---
 sha1_file.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index ec957db5e1..8ce80d4481 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2532,6 +2532,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
while (delta_stack_nr) {
void *delta_data;
void *base = data;
+   void *external_base = NULL;
unsigned long delta_size, base_size = size;
int i;
 
@@ -2558,6 +2559,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
  p->pack_name);
mark_bad_packed_object(p, base_sha1);
base = read_object(base_sha1, , 
_size);
+   external_base = base;
}
}
 
@@ -2576,6 +2578,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
  "at offset %"PRIuMAX" from %s",
  (uintmax_t)curpos, p->pack_name);
data = NULL;
+   free(external_base);
continue;
}
 
@@ -2595,6 +2598,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
error("failed to apply delta");
 
free(delta_data);
+   free(external_base);
}
 
*final_type = type;
-- 
2.12.0

Re: What's cooking in git.git (Feb 2017, #04; Tue, 14)

[René Scharfe]

Am 14.02.2017 um 23:59 schrieb Junio C Hamano:

* rs/ls-files-partial-optim (2017-02-13) 2 commits
 - ls-files: move only kept cache entries in prune_cache()
 - ls-files: pass prefix length explicitly to prune_cache()

 "ls-files" run with pathspec has been micro-optimized to avoid one
 extra call to memmove().


Nit: The number of memmove(3) calls stays the same, but the number of 
bytes that are moved is reduced.


René

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[René Scharfe]

Am 13.02.2017 um 17:23 schrieb Johannes Schindelin:
> Hi René,
> 
> On Fri, 10 Feb 2017, René Scharfe wrote:
> 
>> Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:
>>> It is curious that only MacOSX builds trigger an error about this, both
>>> GCC and Clang, but not Linux GCC nor Clang (see
>>> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
>>>
>>> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
>>>   uninitialized whenever 'if' condition is true
>>>   [-Werror,-Wsometimes-uninitialized]
>>> if (missing_good && !missing_bad && current_term &&
>>> ^~~
>>> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
>>> if (!good_syn)
>>>  ^~~~
>>
>> The only way that good_syn could be used in the if block is by going to the
>> label finish, which does the following before returning:
>>
>>  if (!bad_ref)
>>  free(bad_ref);
>>  if (!good_glob)
>>  free(good_glob);
>>  if (!bad_syn)
>>  free(bad_syn);
>>  if (!good_syn)
>>  free(good_syn);
>>
>> On Linux that code is elided completely -- freeing NULL is a no-op.  I guess
>> free(3) has different attributes on OS X and compilers don't dare to optimize
>> it away there.
>>
>> So instead of calling free(3) only in the case when we did not allocate 
>> memory
>> (which makes no sense and leaks) we should either call it in the opposite
>> case, or (preferred) unconditionally, as it can handle the NULL case itself.
>> Once that's fixed initialization will be required even on Linux.
> 
> Exactly, free(NULL) is a no-op. The problem before this fixup was that
> good_syn was not initialized to NULL.

Strictly speaking: no.  The value doesn't matter -- the free(3) calls
above will be done with NULL regardless, due to the conditionals.
Setting bad_syn and good_syn to an invalid pointer would have calmed
the compiler just as well, and would have had no ill side effect (i.e.
no invalid free(3) call).

Initializing to NULL is still the correct thing to do, of course --
together with removing the conditionals (or at least the negations).

But back to the topic of why the compilers on OS X didn't optimize out
the free(3) calls with their conditionals.  AFAICS no attributes are
set for the function in stdlib.h of in glibc[1] or Darwin[2].  And I
can't see any relevant option in config.mak.uname (e.g. -no-builtin).
It's not terribly important, but does anyone know what prevents the
elision of "if (!p) free(p);" on OS X?

René


[1] 
https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/stdlib.h;h=292c6a2f053a2a578cd09d75307c26ed191e1c00;hb=b987917e6aa7ffe2fd74f0b6a989438e6edd0727
[2] 
https://opensource.apple.com/source/Libc/Libc-1158.30.7/include/stdlib.h.auto.html

Re: What's cooking in git.git (Feb 2017, #03; Fri, 10)

[René Scharfe]

Am 12.02.2017 um 19:32 schrieb Vegard Nossum:

I said I would resubmit the patches with more config options and more
command-line arguments (to avoid potentially breaking backwards
compatibility), but IIRC the response seemed to be "preceding blank line
heuristic is good enough" and "why bother", so I ended up not not
resubmitting anything.


I was (and still am) looking forward to your patches.  The current 
heuristic is simplistic, the patches you already sent improve the output 
in certain scenarios, my proposed changes on top aimed to limit 
drawbacks in other scenarios, but together they still have shortcomings.


Avoiding new switches would be nice, though (if possible).  I feel we 
need a lot more tests to nail down our expectations.


René

[PATCH] rm: reuse strbuf for all remove_dir_recursively() calls, again

[René Scharfe]

Don't throw the memory allocated for remove_dir_recursively() away after
a single call, use it for the other entries as well instead.

This change was done before in deb8e15a (rm: reuse strbuf for all
remove_dir_recursively() calls), but was reverted as a side-effect of
55856a35 (rm: absorb a submodules git dir before deletion). Reinstate
the optimization.

Signed-off-by: Rene Scharfe 
---
Was deb8e15a a rebase victim?

 builtin/rm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 452170a3ab..fb79dcab18 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -360,15 +360,14 @@ int cmd_rm(int argc, const char **argv, const char 
*prefix)
 */
if (!index_only) {
int removed = 0, gitmodules_modified = 0;
+   struct strbuf buf = STRBUF_INIT;
for (i = 0; i < list.nr; i++) {
const char *path = list.entry[i].name;
if (list.entry[i].is_submodule) {
-   struct strbuf buf = STRBUF_INIT;
-
+   strbuf_reset();
strbuf_addstr(, path);
if (remove_dir_recursively(, 0))
die(_("could not remove '%s'"), path);
-   strbuf_release();
 
removed = 1;
if (!remove_path_from_gitmodules(path))
@@ -382,6 +381,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
if (!removed)
die_errno("git rm: '%s'", path);
}
+   strbuf_release();
if (gitmodules_modified)
stage_updated_gitmodules();
}
-- 
2.11.1

Re: [PATCH] cocci: detect useless free(3) calls

[René Scharfe]

Am 11.02.2017 um 20:31 schrieb Lars Schneider:
> how do you run these checks on the entire Git source?
> Do you run each semantic patch file on the source like this?
> 
> spatch --sp-file contrib/coccinelle/qsort.cocci --dir /path/to/git/git
> ...
> spatch --sp-file contrib/coccinelle/free.cocci --dir /path/to/git/git

With "make coccicheck", which runs spatch against the items in the make
variable C_SOURCES, for all .cocci files.

> How stable do you consider these checks? Would it make sense to run them
> as part of the Travis-CI build [1]? 

There seem to have been problems with older versions[2], but I'm not
aware of other issues.

Having these checks run automatically would be nice because they
require a special tool which I guess not everybody is willing (or able)
to install and because they take multiple minutes.

René


[2] https://public-inbox.org/git/014ef44e-9dd8-40b3-a3ec-b483f938e...@web.de/

[PATCH] cocci: detect useless free(3) calls

[René Scharfe]

Add a semantic patch for removing checks that cause free(3) to only be
called with a NULL pointer, as that must be a programming mistake.

Signed-off-by: Rene Scharfe 
---
No cases are found in master or next, but 1d263b93 (bisect--helper:
`bisect_next_check` & bisect_voc shell function in C) introduced four
of them to pu.

 contrib/coccinelle/free.cocci | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
index e28213161a..c03ba737e5 100644
--- a/contrib/coccinelle/free.cocci
+++ b/contrib/coccinelle/free.cocci
@@ -3,3 +3,9 @@ expression E;
 @@
 - if (E)
   free(E);
+
+@@
+expression E;
+@@
+- if (!E)
+  free(E);
-- 
2.11.1

Re: What's cooking in git.git (Feb 2017, #03; Fri, 10)

[René Scharfe]

Am 10.02.2017 um 23:24 schrieb Junio C Hamano:

* vn/xdiff-func-context (2017-01-15) 1 commit
 - xdiff -W: relax end-of-file function detection

 "git diff -W" has been taught to handle the case where a new
 function is added at the end of the file better.

 Will hold.
 Discussion on an follow-up change to go back from the line that
 matches the funcline to show comments before the function
 definition has not resulted in an actionable conclusion.


This one is a bug fix and can be merged already IMHO.


I have raw patches for showing comments before functions with -W, but 
they don't handle the case of a change being within such a comment. 
We'd want to show the trailing function which it is referring to in such 
a case, right?


And that's a bit tricky because it requires a more complicated model: 
Currently we distinguish only between function lines and the rest, while 
the new way would require identifying leading comment lines and probably 
also trailing function body lines in addition to that, and neither can 
be classified simply by looking at the line in question -- their type 
depends on that of neighboring lines.


René

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[René Scharfe]

Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:

It is curious that only MacOSX builds trigger an error about this, both
GCC and Clang, but not Linux GCC nor Clang (see
https://travis-ci.org/git/git/jobs/200182819#L1152 for details):

builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
uninitialized whenever 'if' condition is true
[-Werror,-Wsometimes-uninitialized]
if (missing_good && !missing_bad && current_term &&
^~~
builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
if (!good_syn)
 ^~~~


The only way that good_syn could be used in the if block is by going to 
the label finish, which does the following before returning:


if (!bad_ref)
free(bad_ref);
if (!good_glob)
free(good_glob);
if (!bad_syn)
free(bad_syn);
if (!good_syn)
free(good_syn);

On Linux that code is elided completely -- freeing NULL is a no-op.  I 
guess free(3) has different attributes on OS X and compilers don't dare 
to optimize it away there.


So instead of calling free(3) only in the case when we did not allocate 
memory (which makes no sense and leaks) we should either call it in the 
opposite case, or (preferred) unconditionally, as it can handle the NULL 
case itself.  Once that's fixed initialization will be required even on 
Linux.


René

[PATCH 2/2] ls-files: move only kept cache entries in prune_cache()

[René Scharfe]

prune_cache() first identifies those entries at the start of the sorted
array that can be discarded.  Then it moves the rest of the entries up.
Last it identifies the unwanted trailing entries among the moved ones
and cuts them off.

Change the order: Identify both start *and* end of the range to keep
first and then move only those entries to the top.  The resulting code
is slightly shorter and a bit more efficient.

Signed-off-by: Rene Scharfe 
---
The performance impact is probably only measurable with a *really* big
index.

 builtin/ls-files.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 18105ec7ea..1c0f057d02 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -379,10 +379,7 @@ static void prune_cache(const char *prefix, size_t 
prefixlen)
pos = cache_name_pos(prefix, prefixlen);
if (pos < 0)
pos = -pos-1;
-   memmove(active_cache, active_cache + pos,
-   (active_nr - pos) * sizeof(struct cache_entry *));
-   active_nr -= pos;
-   first = 0;
+   first = pos;
last = active_nr;
while (last > first) {
int next = (last + first) >> 1;
@@ -393,7 +390,9 @@ static void prune_cache(const char *prefix, size_t 
prefixlen)
}
last = next;
}
-   active_nr = last;
+   memmove(active_cache, active_cache + pos,
+   (last - pos) * sizeof(struct cache_entry *));
+   active_nr = last - pos;
 }
 
 /*
-- 
2.11.1

[PATCH 1/2] ls-files: pass prefix length explicitly to prune_cache()

[René Scharfe]

The function prune_cache() relies on the fact that it is only called on
max_prefix and sneakily uses the matching global variable max_prefix_len
directly.  Tighten its interface by passing both the string and its
length as parameters.  While at it move the NULL check into the function
to collect all cache-pruning related logic in one place.

Signed-off-by: Rene Scharfe 
---
Not urgent, of course.

 builtin/ls-files.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1592290815..18105ec7ea 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -369,11 +369,14 @@ static void show_files(struct dir_struct *dir)
 /*
  * Prune the index to only contain stuff starting with "prefix"
  */
-static void prune_cache(const char *prefix)
+static void prune_cache(const char *prefix, size_t prefixlen)
 {
-   int pos = cache_name_pos(prefix, max_prefix_len);
+   int pos;
unsigned int first, last;
 
+   if (!prefix)
+   return;
+   pos = cache_name_pos(prefix, prefixlen);
if (pos < 0)
pos = -pos-1;
memmove(active_cache, active_cache + pos,
@@ -384,7 +387,7 @@ static void prune_cache(const char *prefix)
while (last > first) {
int next = (last + first) >> 1;
const struct cache_entry *ce = active_cache[next];
-   if (!strncmp(ce->name, prefix, max_prefix_len)) {
+   if (!strncmp(ce->name, prefix, prefixlen)) {
first = next+1;
continue;
}
@@ -641,8 +644,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
  show_killed || show_modified || show_resolve_undo))
show_cached = 1;
 
-   if (max_prefix)
-   prune_cache(max_prefix);
+   prune_cache(max_prefix, max_prefix_len);
if (with_tree) {
/*
 * Basic sanity check; show-stages and show-unmerged
-- 
2.11.1

Re: [PATCH] dir: avoid allocation in fill_directory()

[René Scharfe]

Am 08.02.2017 um 07:22 schrieb Duy Nguyen:

On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe <l@web.de> wrote:

Pass the match member of the first pathspec item directly to
read_directory() instead of using common_prefix() to duplicate it first,
thus avoiding memory duplication, strlen(3) and free(3).


How about killing common_prefix()? There are two other callers in
ls-files.c and commit.c and it looks safe to do (but I didn't look
very hard).


I would like that, but it doesn't look like it's worth it.  I have a 
patch series for making overlay_tree_on_cache() take pointer+length, but 
it's surprisingly long and bloats the code.  Duplicating a small piece 
of memory once per command doesn't look so bad in comparison.


(The payoff for avoiding an allocation is higher for library functions 
like fill_directory().)


But while working on that I found two opportunities for improvement in 
prune_cache().  I'll send patches shortly.



There's a subtle difference. Before the patch, prefix[prefix_len] is
NUL. After the patch, it's not always true. If some code (incorrectly)
depends on that, this patch exposes it. I had a look inside
read_directory() though and it looks like no such code exists. So, all
good.


Thanks for checking.

NB: The code before 966de302 (dir: convert fill_directory to use the 
pathspec struct interface, committed 2017-01-04) made the same 
assumption, i.e. that NUL is not needed.


René

Re: Trying to use xfuncname without success.

[René Scharfe]

Am 09.02.2017 um 01:10 schrieb Jack Adrian Zappa:

 where it has grabbed a line at 126 and is using that for the hunk header.


When I say that, I mean that it is using that line for *every* hunk
header, for every change, regardless if it has passed a hunk head that
it should have matched.


Strange.  That should only happen if no other function lines are 
recognized before the changes.  You can check if that's the case using 
git grep and your xfuncline regex, e.g. like this:



  $ git grep -En "^[\t ]*

Re: Trying to use xfuncname without success.

[René Scharfe]

Am 08.02.2017 um 18:11 schrieb Jack Adrian Zappa:
> Thanks Rene, but you seem to have missed the point.  NOTHING is
> working.  No matter what I put there, it doesn't seem to get matched.

I'm not so sure about that.  With your example I get this diff without
setting diff.natvis.xfuncname:

diff --git a/a.natvis b/a.natvis
index 7f9bdf5..bc3c090 100644
--- a/a.natvis
+++ b/a.natvis
@@ -19,7 +19,7 @@ 
xmlns="http://schemas.microsoft.com/vstudio/debugger/natvis/2010;>
 
 
   
-  added_var
+  added_vars
 
 
   var2

Note the XML namespace in the hunk header.  It's put there by the
default rule because "xmlns" starts at the beginning of the line.  Your
diff has nothing there, which means the default rule is not used, i.e.
your user-defined rule is in effect.

Come to think of it, this line break in the middle of the AutoVisualizer
tab might have been added by your email client unintentionally, so that
we use different test files, which then of course results in different
diffs.  Is that the case?

Anyway, if I run the following two commands:

$ git config diff.natvis.xfuncname "^[\t ]*.gitattributes

... then I get this, both on Linux (git version 2.11.1) and on Windows
(git version 2.11.1.windows.1):

diff --git a/a.natvis b/a.natvis
index 7f9bdf5..bc3c090 100644
--- a/a.natvis
+++ b/a.natvis
@@ -19,7 +19,7 @@ test
 
 
   
-  added_var
+  added_vars
 
 
   var2

> Just to be sure, I tested your regex and again it didn't work.

At this point I'm out of ideas, sorry. :(  The only way I was able to
break it was due to mistyping the extension as "netvis" several times
for some reason.

René

Re: Trying to use xfuncname without success.

[René Scharfe]

Am 07.02.2017 um 20:21 schrieb Jack Adrian Zappa:

I'm trying to setup a hunk header for .natvis files. For some reason,
it doesn't seem to be working. I'm following their instructions from
here, which doesn't say much in terms of restrictions of the regex,
such as, is the matched item considered the hunk header or do I need a
group? I have tried both with no success. This is what I have:

[diff "natvis"]
xfuncname = "^[\\\t ]*

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 01.02.2017 um 19:33 schrieb Junio C Hamano:
> René Scharfe <l@web.de> writes:
> 
>> Size checks could be added to SIMPLE_SWAP as well.
> 
> Between SWAP() and SIMPLE_SWAP() I am undecided.
> 
> If the compiler can infer the type and the size of the two
> "locations" given to the macro, there is no technical reason to
> require the caller to specify the type as an extra argument, so
> SIMPLE_SWAP() may not necessarily an improvement over SWAP() from
> that point of view.  If the redundancy is used as a sanity check,
> I'd be in favor of SIMPLE_SWAP(), though.
> 
> If the goal of SIMPLE_SWAP(), on the other hand, were to support the
> "only swap char with int for small value" example earlier in the
> thread, it means you cannot sanity check the type of things being
> swapped in the macro, and the readers of the code need to know about
> the details of what variables are being swapped.  It looks to me
> that it defeats the main benefit of using a macro.

Full type inference could be done with C11's _Generic for basic types,
while typeof would be needed for complex ones, I guess.  Checking that
sizes match is better than nothing and portable to ancient platforms,
though.  Having an explicit type given is portable and easy to use for
checks, of course, e.g. like this:

#define SIMPLE_SWAP(T, a, b) do { \
T swap_tmp_ = a + BUILD_ASSERT_OR_ZERO(sizeof(T) == sizeof(a)); \
a = b + BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)); \
b = swap_tmp_; \
} while(0)

It doesn't support expressions with side effects, but that's probably
not much of a concern.

Swapping between different types would then still have to be done
manually, but I wonder how common that is -- couldn't find such a case
in our tree.

René

[PATCH] dir: avoid allocation in fill_directory()

[René Scharfe]

Pass the match member of the first pathspec item directly to
read_directory() instead of using common_prefix() to duplicate it first,
thus avoiding memory duplication, strlen(3) and free(3).

Signed-off-by: Rene Scharfe 
---
This updates 966de3028 (dir: convert fill_directory to use the pathspec
struct interface).  The result is closer to the original, yet still
using the modern interface.

This patch eerily resembles 2b189435 (dir: simplify fill_directory()).

 dir.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index 65c3e681b8..4541f9e146 100644
--- a/dir.c
+++ b/dir.c
@@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec)
 
 int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
 {
-   char *prefix;
+   const char *prefix;
size_t prefix_len;
 
/*
 * Calculate common prefix for the pathspec, and
 * use that to optimize the directory walk
 */
-   prefix = common_prefix(pathspec);
-   prefix_len = prefix ? strlen(prefix) : 0;
+   prefix_len = common_prefix_len(pathspec);
+   prefix = prefix_len ? pathspec->items[0].match : "";
 
/* Read the directory and prune it */
read_directory(dir, prefix, prefix_len, pathspec);
 
-   free(prefix);
return prefix_len;
 }
 
-- 
2.11.1

[PATCH] p5302: create repositories for index-pack results explicitly

[René Scharfe]

Before 7176a314 (index-pack: complain when --stdin is used outside of a
repo) index-pack silently created a non-existing target directory; now
the command refuses to work unless it's used against a valid repository.
That causes p5302 to fail, which relies on the former behavior.  Fix it
by setting up the destinations for its performance tests using git init.

Signed-off-by: Rene Scharfe 
---
 t/perf/p5302-pack-index.sh | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh
index 5ee9211f98..99bdb16c85 100755
--- a/t/perf/p5302-pack-index.sh
+++ b/t/perf/p5302-pack-index.sh
@@ -13,6 +13,13 @@ test_expect_success 'repack' '
export PACK
 '
 
+test_expect_success 'create target repositories' '
+   for repo in t1 t2 t3 t4 t5 t6
+   do
+   git init --bare $repo
+   done
+'
+
 test_perf 'index-pack 0 threads' '
GIT_DIR=t1 git index-pack --threads=1 --stdin < $PACK
 '
-- 
2.11.1

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 01.02.2017 um 12:47 schrieb Jeff King:

I'm not altogether convinced that SWAP() is an improvement in
readability. I really like that it's shorter than the code it replaces,
but there is a danger with introducing opaque constructs. It's one more
thing for somebody familiar with C but new to the project to learn or
get wrong.


I'm biased, of course, but SIMPLE_SWAP and SWAP exchange the values of 
two variables -- how could someone get that wrong?  Would it help to 
name the macro SWAP_VALUES?  Or XCHG? ;)



IMHO the main maintenance gain from René's patch is that you don't have
to specify the type, which means you can never have a memory-overflow
bug due to incorrect types. If we lose that, then I don't see all that
much value in the whole thing.


Size checks could be added to SIMPLE_SWAP as well.

The main benefit of a swap macro is reduced repetition IMHO: Users 
specify the variables to swap once instead of twice in a special order, 
and with SWAP they don't need to declare their type again.  Squeezing 
out redundancy makes the code easier to read and modify.


René

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 30.01.2017 um 23:21 schrieb Brandon Williams:

On 01/30, René Scharfe wrote:

Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:

It is curious, though, that an
expression like "sizeof(a++)" would not be rejected.


Clang normally warns about something like this ("warning: expression
with side effects has no effect in an unevaluated context
[-Wunevaluated-expression]"), but not if the code is part of a
macro.  I don't know if that's intended, but it sure is helpful in
the case of SWAP.


Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?


That might be a valid expectation, but GCC says "error: lvalue
required as unary '&' operand" and clang puts it "error: cannot take
the address of an rvalue of type".

René


Perhaps we could disallow a side-effect operator in the macro.  By
disallow I mean place a comment at the definition to the macro and
hopefully catch something like that in code-review.  We have the same
issue with the `ALLOC_GROW()` macro.


SWAP(a++, ...) is caught by the compiler, SWAP(*a++, ...) works fine. 
Technically that should be enough. :)  A comment wouldn't hurt, of course.


René

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 31.01.2017 um 13:13 schrieb Johannes Schindelin:

Hi René,

On Mon, 30 Jan 2017, René Scharfe wrote:


Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:


The commit you quoted embarrasses me, and I have no excuse for it. I
would love to see that myswap() ugliness fixed by replacing it with a
construct that is simpler, and generates good code even without any
smart compiler.


I don't see a way to do that without adding a type parameter.


Exactly. And you know what? I would be very okay with that type parameter.

Coccinelle [*1*] should be able to cope with that, too, mehtinks.


Yes, a semantic patch can turn the type of the temporary variable into a 
macro parameter.  Programmers would have to type the type, though, 
making the macro only half as good.



It would be trivially "optimized" out of the box, even when compiling with
Tiny C or in debug mode.


Such a compiler is already slowed down by memset(3) calls for 
initializing objects and lack of other optimizations.  I doubt a few 
more memcpy(3) calls would make that much of a difference.


NB: git as compiled with TCC fails several tests, alas.  Builds wickedly 
fast, though.



And it would even allow things like this:

#define SIMPLE_SWAP(T, a, b) do { T tmp_ = a; a = b; b = tmp_; } while (0)
...
uint32_t large;
char nybble;

...

if (!(large & ~0xf)) {
SIMPLE_SWAP(char, nybble, large);
...
}

i.e. mixing types, when possible.

And while I do not necessarily expect that we need anything like this
anytime soon, merely the fact that it allows for this flexibility, while
being very readable at the same time, would make it a pretty good design
in my book.


Such a skinny macro which only hides repetition is kind of attractive 
due to its simplicity; I can't say the same about the mixed type example 
above, though.


The fat version isn't that bad either even without inlining, includes a 
few safety checks and doesn't require us to tell the compiler something 
it already knows very well.  I'd rather let the machine do the work.


René

Re: [PATCH 3/5] use SWAP macro

[René Scharfe]

Am 30.01.2017 um 23:22 schrieb Junio C Hamano:

René Scharfe <l@web.de> writes:


if (tree2->flags & UNINTERESTING) {
-   struct object *tmp = tree2;
-   tree2 = tree1;
-   tree1 = tmp;
+   SWAP(tree2, tree1);


A human would have written this SWAP(tree1, tree2).

Not that I think such a manual fix-up should be made in _this_
patch, which may end up mixing mechanical conversion (which we may
want to keep reproducible) and hand tweaks.  But this swapped swap
reads somewhat silly.


Well, a human wrote "tmp = tree2" -- sometimes one likes to count down 
instead of up, I guess.


We can make Coccinelle order the parameters alphabetically, but I don't 
know how to do so without duplicating most of the semantic patch.  And 
thats would result in e.g. SWAP(new_tree, old_tree), which might be odd 
as well.


I'd just leave them in whatever order they were, but that's probably 
because I'm lazy.


René

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:

It is curious, though, that an
expression like "sizeof(a++)" would not be rejected.


Clang normally warns about something like this ("warning: expression 
with side effects has no effect in an unevaluated context 
[-Wunevaluated-expression]"), but not if the code is part of a macro.  I 
don't know if that's intended, but it sure is helpful in the case of SWAP.



Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?


That might be a valid expectation, but GCC says "error: lvalue required 
as unary '&' operand" and clang puts it "error: cannot take the address 
of an rvalue of type".


René

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:

So I tried to verify that Visual C optimizes this well, and oh my deity,
this was not easy. In Debug mode, it does not optimize, i.e. the memcpy()
will be called, even for simple 32-bit integers. In Release mode, Visual
Studio's defaults turn on "whole-program optimization" which means that
the only swapping that is going on in the end is that the meaning of two
registers is swapped, i.e. the SWAP() macro is expanded to... no assembler
code at all.

After trying this and that and something else, I finally ended up with the
file-scope optimized SWAP() resulting in this assembly code:

7FF791311000  mov eax,dword ptr [rcx]
7FF791311002  mov r8d,dword ptr [rdx]
7FF791311005  mov dword ptr [rcx],r8d
7FF791311008  mov dword ptr [rdx],eax


This looks good.


However, recent events (including some discussions on this mailing list
which had unfortunate consequences) made me trust my instincts more. And
my instincts tell me that it would be unwise to replace code that swaps
primitive types in the straight-forward way by code that relies on
advanced compiler optimization to generate efficient code, otherwise
producing very suboptimal code.


I don't know how difficult it was to arrive at the result above, but I 
wouldn't call inlining memcpy(3) an advanced optimization (anymore?), 
since the major compilers seem to be doing that.


The SWAP in prio-queue.c seems to be the one with the biggest 
performance impact.  Or perhaps it's the one in lookup_object()?  The 
former is easier to measure, though.


Here's what I get with CFLAGS="-builtin -O2" (best of five):

$ time ./t/helper/test-prio-queue $(seq 10 -1 1) dump >/dev/null

real0m0.142s
user0m0.120s
sys 0m0.020s

And this is with CFLAGS="-no-builtin -O2":

$ time ./t/helper/test-prio-queue $(seq 10 -1 1) dump >/dev/null

real0m0.170s
user0m0.156s
sys 0m0.012s

Hmm.  Not nice, but also not prohibitively slow.


The commit you quoted embarrasses me, and I have no excuse for it. I would
love to see that myswap() ugliness fixed by replacing it with a construct
that is simpler, and generates good code even without any smart compiler.


I don't see a way to do that without adding a type parameter.

René

Re: [PATCH 5/5] graph: use SWAP macro

[René Scharfe]

Am 30.01.2017 um 17:16 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


Exchange the values of graph->columns and graph->new_columns using the
macro SWAP instead of hand-rolled code.  The result is shorter and
easier to read.

This transformation was not done by the semantic patch swap.cocci
because there's an unrelated statement between the second and the last
step of the exchange, so it didn't match the expected pattern.


Is it really true that Coccinelle cannot be told to look for a code block
that declares a variable that is then used *only* in the lines we want to
match and replace?


Hope I parsed your question correctly; my answer would be that it can't 
be true because that's basically what the proposed semantic patch does:


@ swap @
type T;
T tmp, a, b;
@@
- tmp = a;
- a = b;
- b = tmp;
+ SWAP(a, b);

@ extends swap @
identifier unused;
@@
  {
  ...
- T unused;
  ... when != unused
  }

The first part (up to the "+") looks for a opportunities to use SWAP, 
and the second part looks for blocks where that transformation was done 
and we declare identifiers that are/became unused.


It did not match the code in graph.c because the pattern was basically:

tmp = a;
a = b;
something = totally_different;
b = tmp;

Coccinelle can be told to ignore such unrelated code by adding "... when 
!= tmp" etc. (which matches context lines that don't reference tmp), but 
that's slooow.  (Perhaps I just did it wrong, though.)


René

Re: [PATCH 3/5] use SWAP macro

[René Scharfe]

Am 30.01.2017 um 17:03 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 806dd7a885..8ce00480cd 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -147,9 +147,7 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
tree1 = opt->pending.objects[0].item;
tree2 = opt->pending.objects[1].item;
if (tree2->flags & UNINTERESTING) {
-   struct object *tmp = tree2;
-   tree2 = tree1;
-   tree1 = tmp;
+   SWAP(tree2, tree1);
}


Is there a way to transform away the curly braces for blocks that become
single-line blocks, too?


Interesting question.  I guess this can be done by using a Python script 
(see contrib/coccinelle/strbuf.cocci for an example).  I'll leave this 
as homework for readers interested in Coccinelle, at least for a while. :)


René

Re: [PATCH 4/5] diff: use SWAP macro

[René Scharfe]

Am 30.01.2017 um 17:04 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


Use the macro SWAP to exchange the value of pairs of variables instead
of swapping them manually with the help of a temporary variable.  The
resulting code is shorter and easier to read.

The two cases were not transformed by the semantic patch swap.cocci
because it's extra careful and handles only cases where the types of all
variables are the same -- and here we swap two ints and use an unsigned
temporary variable for that.  Nevertheless the conversion is safe, as
the value range is preserved with and without the patch.


One way to make this more obvious would be to change the type to signed
first, and then transform (which then would catch these cases too,
right?).


I'm not sure it would be more obvious, but it would certainly make the 
type change more explicit.  In diff-index.c we might even want to change 
the type of the swapped values from int to unsigned, which is more 
fitting for file modes.  In diff.c we'd need to add a separate variable, 
as tmp is shared with other (unsigned) swaps.


René

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 30.01.2017 um 17:01 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092b..66cd466eea 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char 
*suffix)
return strip_suffix(str, suffix, );
 }

+#define SWAP(a, b) do {\
+   void *_swap_a_ptr = &(a);   \
+   void *_swap_b_ptr = &(b);   \
+   unsigned char _swap_buffer[sizeof(a)];  \
+   memcpy(_swap_buffer, _swap_a_ptr, sizeof(a));   \
+   memcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\
+  BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));   \
+   memcpy(_swap_b_ptr, _swap_buffer, sizeof(a));   \
+} while (0)
+
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)


It may seem as a matter of taste, or maybe not: I prefer this without the
_swap_a_ptr (and I would also prefer not to use identifiers starting with
an underscore, as section 7.1.3 Reserved Identifiers of the C99 standard
says they are reserved):

+#define SWAP(a, b) do {\
+   unsigned char swap_buffer_[sizeof(a)];  \
+   memcpy(swap_buffer_, &(a), sizeof(a));  \
+   memcpy(&(a), &(b), sizeof(a) +  \
+  BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));   \
+   memcpy(&(b), swap_buffer_, sizeof(a));  \
+} while (0)


We can move the underscore to the end, but using a and b directly will 
give surprising results if the parameters have side effects.  E.g. if 
you want to swap the first two elements of two arrays you might want to 
do this:


SWAP(*x++, *y++);
SWAP(*x++, *y++);

And that would increment twice as much as one would guess and access 
unexpected elements.



One idea to address the concern that not all C compilers people use to
build Git may optimize away those memcpy()s: we could also introduce a
SWAP_PRIMITIVE_TYPE (or SWAP2 or SIMPLE_SWAP or whatever) that accepts
only primitive types. But since __typeof__() is not portable...


I wouldn't worry too much about such a solution before seeing that SWAP 
(even with memcpy(3) -- this function is probably optimized quite 
heavily on most platforms) causes an actual performance problem.


René

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 30.01.2017 um 16:39 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


Add a macro for exchanging the values of variables.  It allows users to
avoid repetition and takes care of the temporary variable for them.  It
also makes sure that the storage sizes of its two parameters are the
same.  Its memcpy(1) calls are optimized away by current compilers.


How certain are we that "current compilers" optimize that away? And about
which "current compilers" are we talking? GCC? GCC 6? Clang? Clang 3?
Clang 3.8.*? Visual C 2005+?


GCC 4.4.7 and clang 3.2 on x86-64 compile the macro to the same object 
code as a manual swap ; see https://godbolt.org/g/F4b9M9.  Both were 
released 2012.  That website doesn't offer Visual C(++), but since you 
added the original macro in e5a94313c0 ("Teach git-apply about '-R'", 
2006) I guess we have that part covered. ;)


René

[PATCH] receive-pack: call string_list_clear() unconditionally

[René Scharfe]

string_list_clear() handles empty lists just fine, so remove the
redundant check.

Signed-off-by: Rene Scharfe 
---
 builtin/receive-pack.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 6b97cbdbe..1dbb8a069 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1942,8 +1942,7 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
run_receive_hook(commands, "post-receive", 1,
 _options);
run_update_post_hook(commands);
-   if (push_options.nr)
-   string_list_clear(_options, 0);
+   string_list_clear(_options, 0);
if (auto_gc) {
const char *argv_gc_auto[] = {
"gc", "--auto", "--quiet", NULL,
-- 
2.11.0

[PATCH] use oid_to_hex_r() for converting struct object_id hashes to hex strings

[René Scharfe]

Patch generated by Coccinelle and contrib/coccinelle/object_id.cocci.

Signed-off-by: Rene Scharfe 
---
 builtin/blame.c   | 4 ++--
 builtin/merge-index.c | 2 +-
 builtin/rev-list.c| 2 +-
 diff.c| 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 126b8c9e5b..cffc626540 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1901,7 +1901,7 @@ static void emit_porcelain(struct scoreboard *sb, struct 
blame_entry *ent,
struct origin *suspect = ent->suspect;
char hex[GIT_SHA1_HEXSZ + 1];
 
-   sha1_to_hex_r(hex, suspect->commit->object.oid.hash);
+   oid_to_hex_r(hex, >commit->object.oid);
printf("%s %d %d %d\n",
   hex,
   ent->s_lno + 1,
@@ -1941,7 +1941,7 @@ static void emit_other(struct scoreboard *sb, struct 
blame_entry *ent, int opt)
int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
 
get_commit_info(suspect->commit, , 1);
-   sha1_to_hex_r(hex, suspect->commit->object.oid.hash);
+   oid_to_hex_r(hex, >commit->object.oid);
 
cp = nth_line(sb, ent->lno);
for (cnt = 0; cnt < ent->num_lines; cnt++) {
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index ce356b1da1..2d1b6db6bd 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -22,7 +22,7 @@ static int merge_entry(int pos, const char *path)
if (strcmp(ce->name, path))
break;
found++;
-   sha1_to_hex_r(hexbuf[stage], ce->oid.hash);
+   oid_to_hex_r(hexbuf[stage], >oid);
xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", 
ce->ce_mode);
arguments[stage] = hexbuf[stage];
arguments[stage + 4] = ownbuf[stage];
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index c43decda70..0aa93d5891 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -237,7 +237,7 @@ static int show_bisect_vars(struct rev_list_info *info, int 
reaches, int all)
cnt = reaches;
 
if (revs->commits)
-   sha1_to_hex_r(hex, revs->commits->item->object.oid.hash);
+   oid_to_hex_r(hex, >commits->item->object.oid);
 
if (flags & BISECT_SHOW_ALL) {
traverse_commit_list(revs, show_commit, show_object, info);
diff --git a/diff.c b/diff.c
index f08cd8e033..d91a344908 100644
--- a/diff.c
+++ b/diff.c
@@ -3015,7 +3015,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
if (!one->oid_valid)
sha1_to_hex_r(temp->hex, null_sha1);
else
-   sha1_to_hex_r(temp->hex, one->oid.hash);
+   oid_to_hex_r(temp->hex, >oid);
/* Even though we may sometimes borrow the
 * contents from the work tree, we always want
 * one->mode.  mode is trustworthy even when
-- 
2.11.0

[PATCH] checkout: convert post_checkout_hook() to struct object_id

[René Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/checkout.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfe685c198..80d5e38981 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -56,8 +56,8 @@ static int post_checkout_hook(struct commit *old, struct 
commit *new,
  int changed)
 {
return run_hook_le(NULL, "post-checkout",
-  sha1_to_hex(old ? old->object.oid.hash : null_sha1),
-  sha1_to_hex(new ? new->object.oid.hash : null_sha1),
+  oid_to_hex(old ? >object.oid : _oid),
+  oid_to_hex(new ? >object.oid : _oid),
   changed ? "1" : "0", NULL);
/* "new" can be NULL when checking out from the index before
   a commit exists. */
-- 
2.11.0

[PATCH] use oidcpy() for copying hashes between instances of struct object_id

[René Scharfe]

Patch generated by Coccinelle and contrib/coccinelle/object_id.cocci.

Signed-off-by: Rene Scharfe 
---
 refs/files-backend.c | 2 +-
 wt-status.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f9023939d5..8ee2aba39f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -697,7 +697,7 @@ static int cache_ref_iterator_peel(struct ref_iterator 
*ref_iterator,
 
if (peel_entry(entry, 0))
return -1;
-   hashcpy(peeled->hash, entry->u.value.peeled.hash);
+   oidcpy(peeled, >u.value.peeled);
return 0;
 }
 
diff --git a/wt-status.c b/wt-status.c
index a715e71906..a05328dc48 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -628,7 +628,7 @@ static void wt_status_collect_changes_initial(struct 
wt_status *s)
d->index_status = DIFF_STATUS_ADDED;
/* Leave {mode,oid}_head zero for adds. */
d->mode_index = ce->ce_mode;
-   hashcpy(d->oid_index.hash, ce->oid.hash);
+   oidcpy(>oid_index, >oid);
}
}
 }
@@ -2096,7 +2096,7 @@ static void wt_porcelain_v2_print_unmerged_entry(
if (strcmp(ce->name, it->string) || !stage)
break;
stages[stage - 1].mode = ce->ce_mode;
-   hashcpy(stages[stage - 1].oid.hash, ce->oid.hash);
+   oidcpy([stage - 1].oid, >oid);
sum |= (1 << (stage - 1));
}
if (sum != d->stagemask)
-- 
2.11.0

[PATCH 5/5] graph: use SWAP macro

[René Scharfe]

Exchange the values of graph->columns and graph->new_columns using the
macro SWAP instead of hand-rolled code.  The result is shorter and
easier to read.

This transformation was not done by the semantic patch swap.cocci
because there's an unrelated statement between the second and the last
step of the exchange, so it didn't match the expected pattern.

Signed-off-by: Rene Scharfe 
---
 graph.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/graph.c b/graph.c
index 4c722303d2..29b0f51dc5 100644
--- a/graph.c
+++ b/graph.c
@@ -463,7 +463,6 @@ static void graph_update_width(struct git_graph *graph,
 static void graph_update_columns(struct git_graph *graph)
 {
struct commit_list *parent;
-   struct column *tmp_columns;
int max_new_columns;
int mapping_idx;
int i, seen_this, is_commit_in_columns;
@@ -476,11 +475,8 @@ static void graph_update_columns(struct git_graph *graph)
 * We'll re-use the old columns array as storage to compute the new
 * columns list for the commit after this one.
 */
-   tmp_columns = graph->columns;
-   graph->columns = graph->new_columns;
+   SWAP(graph->columns, graph->new_columns);
graph->num_columns = graph->num_new_columns;
-
-   graph->new_columns = tmp_columns;
graph->num_new_columns = 0;
 
/*
-- 
2.11.0

[PATCH 4/5] diff: use SWAP macro

[René Scharfe]

Use the macro SWAP to exchange the value of pairs of variables instead
of swapping them manually with the help of a temporary variable.  The
resulting code is shorter and easier to read.

The two cases were not transformed by the semantic patch swap.cocci
because it's extra careful and handles only cases where the types of all
variables are the same -- and here we swap two ints and use an unsigned
temporary variable for that.  Nevertheless the conversion is safe, as
the value range is preserved with and without the patch.

Signed-off-by: Rene Scharfe 
---
 diff-no-index.c | 3 +--
 diff.c  | 4 +---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 1ae09894d7..df762fd0f7 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -185,8 +185,7 @@ static int queue_diff(struct diff_options *o,
struct diff_filespec *d1, *d2;
 
if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
-   unsigned tmp;
-   tmp = mode1; mode1 = mode2; mode2 = tmp;
+   SWAP(mode1, mode2);
SWAP(name1, name2);
}
 
diff --git a/diff.c b/diff.c
index 9de1ba264f..6c4f3f6b72 100644
--- a/diff.c
+++ b/diff.c
@@ -5117,11 +5117,9 @@ void diff_change(struct diff_options *options,
return;
 
if (DIFF_OPT_TST(options, REVERSE_DIFF)) {
-   unsigned tmp;
SWAP(old_mode, new_mode);
SWAP(old_sha1, new_sha1);
-   tmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;
-   new_sha1_valid = tmp;
+   SWAP(old_sha1_valid, new_sha1_valid);
SWAP(old_dirty_submodule, new_dirty_submodule);
}
 
-- 
2.11.0

[PATCH 3/5] use SWAP macro

[René Scharfe]

Apply the semantic patch swap.cocci to convert hand-rolled swaps to use
the macro SWAP.  The resulting code is shorter and easier to read, the
object code is effectively unchanged.

The patch for object.c had to be hand-edited in order to preserve the
comment before the change; Coccinelle tried to eat it for some reason.

Signed-off-by: Rene Scharfe 
---
 builtin/diff-tree.c | 4 +---
 builtin/diff.c  | 9 +++--
 diff-no-index.c | 3 +--
 diff.c  | 8 +++-
 graph.c | 5 +
 line-range.c| 3 +--
 merge-recursive.c   | 5 +
 object.c| 4 +---
 pack-revindex.c | 5 +
 prio-queue.c| 4 +---
 strbuf.h| 4 +---
 11 files changed, 15 insertions(+), 39 deletions(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 806dd7a885..8ce00480cd 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -147,9 +147,7 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
tree1 = opt->pending.objects[0].item;
tree2 = opt->pending.objects[1].item;
if (tree2->flags & UNINTERESTING) {
-   struct object *tmp = tree2;
-   tree2 = tree1;
-   tree1 = tmp;
+   SWAP(tree2, tree1);
}
diff_tree_sha1(tree1->oid.hash,
   tree2->oid.hash,
diff --git a/builtin/diff.c b/builtin/diff.c
index 7f91f6d226..3d64b85337 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -45,12 +45,9 @@ static void stuff_change(struct diff_options *opt,
return;
 
if (DIFF_OPT_TST(opt, REVERSE_DIFF)) {
-   unsigned tmp;
-   const unsigned char *tmp_u;
-   const char *tmp_c;
-   tmp = old_mode; old_mode = new_mode; new_mode = tmp;
-   tmp_u = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_u;
-   tmp_c = old_name; old_name = new_name; new_name = tmp_c;
+   SWAP(old_mode, new_mode);
+   SWAP(old_sha1, new_sha1);
+   SWAP(old_name, new_name);
}
 
if (opt->prefix &&
diff --git a/diff-no-index.c b/diff-no-index.c
index f420786039..1ae09894d7 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -186,9 +186,8 @@ static int queue_diff(struct diff_options *o,
 
if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
unsigned tmp;
-   const char *tmp_c;
tmp = mode1; mode1 = mode2; mode2 = tmp;
-   tmp_c = name1; name1 = name2; name2 = tmp_c;
+   SWAP(name1, name2);
}
 
d1 = noindex_filespec(name1, mode1);
diff --git a/diff.c b/diff.c
index f08cd8e033..9de1ba264f 100644
--- a/diff.c
+++ b/diff.c
@@ -5118,13 +5118,11 @@ void diff_change(struct diff_options *options,
 
if (DIFF_OPT_TST(options, REVERSE_DIFF)) {
unsigned tmp;
-   const unsigned char *tmp_c;
-   tmp = old_mode; old_mode = new_mode; new_mode = tmp;
-   tmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;
+   SWAP(old_mode, new_mode);
+   SWAP(old_sha1, new_sha1);
tmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;
new_sha1_valid = tmp;
-   tmp = old_dirty_submodule; old_dirty_submodule = 
new_dirty_submodule;
-   new_dirty_submodule = tmp;
+   SWAP(old_dirty_submodule, new_dirty_submodule);
}
 
if (options->prefix &&
diff --git a/graph.c b/graph.c
index d4e8519c90..4c722303d2 100644
--- a/graph.c
+++ b/graph.c
@@ -997,7 +997,6 @@ static void graph_output_post_merge_line(struct git_graph 
*graph, struct strbuf
 static void graph_output_collapsing_line(struct git_graph *graph, struct 
strbuf *sb)
 {
int i;
-   int *tmp_mapping;
short used_horizontal = 0;
int horizontal_edge = -1;
int horizontal_edge_target = -1;
@@ -1132,9 +1131,7 @@ static void graph_output_collapsing_line(struct git_graph 
*graph, struct strbuf
/*
 * Swap mapping and new_mapping
 */
-   tmp_mapping = graph->mapping;
-   graph->mapping = graph->new_mapping;
-   graph->new_mapping = tmp_mapping;
+   SWAP(graph->mapping, graph->new_mapping);
 
/*
 * If graph->mapping indicates that all of the branch lines
diff --git a/line-range.c b/line-range.c
index de4e32f942..323399d16c 100644
--- a/line-range.c
+++ b/line-range.c
@@ -269,8 +269,7 @@ int parse_range_arg(const char *arg, nth_line_fn_t 
nth_line_cb,
return -1;
 
if (*begin && *end && *end < *begin) {
-   long tmp;
-   tmp = *end; *end = *begin; *begin = tmp;
+   SWAP(*end, *begin);
}
 
return 0;
diff --git a/merge-recursive.c b/merge-recursive.c
index 

[PATCH 2/5] apply: use SWAP macro

[René Scharfe]

Use the exported macro SWAP instead of the file-scoped macro swap and
remove the latter's definition.

Signed-off-by: Rene Scharfe 
---
 apply.c | 23 +++
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/apply.c b/apply.c
index 2ed808d429..0e2caeab9c 100644
--- a/apply.c
+++ b/apply.c
@@ -2187,29 +2187,20 @@ static int parse_chunk(struct apply_state *state, char 
*buffer, unsigned long si
return offset + hdrsize + patchsize;
 }
 
-#define swap(a,b) myswap((a),(b),sizeof(a))
-
-#define myswap(a, b, size) do {\
-   unsigned char mytmp[size];  \
-   memcpy(mytmp, , size);\
-   memcpy(, , size);   \
-   memcpy(, mytmp, size);\
-} while (0)
-
 static void reverse_patches(struct patch *p)
 {
for (; p; p = p->next) {
struct fragment *frag = p->fragments;
 
-   swap(p->new_name, p->old_name);
-   swap(p->new_mode, p->old_mode);
-   swap(p->is_new, p->is_delete);
-   swap(p->lines_added, p->lines_deleted);
-   swap(p->old_sha1_prefix, p->new_sha1_prefix);
+   SWAP(p->new_name, p->old_name);
+   SWAP(p->new_mode, p->old_mode);
+   SWAP(p->is_new, p->is_delete);
+   SWAP(p->lines_added, p->lines_deleted);
+   SWAP(p->old_sha1_prefix, p->new_sha1_prefix);
 
for (; frag; frag = frag->next) {
-   swap(frag->newpos, frag->oldpos);
-   swap(frag->newlines, frag->oldlines);
+   SWAP(frag->newpos, frag->oldpos);
+   SWAP(frag->newlines, frag->oldlines);
}
}
 }
-- 
2.11.0

[PATCH 1/5] add SWAP macro

[René Scharfe]

Add a macro for exchanging the values of variables.  It allows users
to avoid repetition and takes care of the temporary variable for them.
It also makes sure that the storage sizes of its two parameters are the
same.  Its memcpy(1) calls are optimized away by current compilers.

Also add a conservative semantic patch for transforming only swaps of
variables of the same type.

Signed-off-by: Rene Scharfe 
---
 contrib/coccinelle/swap.cocci | 28 
 git-compat-util.h | 10 ++
 2 files changed, 38 insertions(+)
 create mode 100644 contrib/coccinelle/swap.cocci

diff --git a/contrib/coccinelle/swap.cocci b/contrib/coccinelle/swap.cocci
new file mode 100644
index 00..a0934d1fda
--- /dev/null
+++ b/contrib/coccinelle/swap.cocci
@@ -0,0 +1,28 @@
+@ swap_with_declaration @
+type T;
+identifier tmp;
+T a, b;
+@@
+- T tmp = a;
++ T tmp;
++ tmp = a;
+  a = b;
+  b = tmp;
+
+@ swap @
+type T;
+T tmp, a, b;
+@@
+- tmp = a;
+- a = b;
+- b = tmp;
++ SWAP(a, b);
+
+@ extends swap @
+identifier unused;
+@@
+  {
+  ...
+- T unused;
+  ... when != unused
+  }
diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092b..66cd466eea 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char 
*suffix)
return strip_suffix(str, suffix, );
 }
 
+#define SWAP(a, b) do {\
+   void *_swap_a_ptr = &(a);   \
+   void *_swap_b_ptr = &(b);   \
+   unsigned char _swap_buffer[sizeof(a)];  \
+   memcpy(_swap_buffer, _swap_a_ptr, sizeof(a));   \
+   memcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\
+  BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));   \
+   memcpy(_swap_b_ptr, _swap_buffer, sizeof(a));   \
+} while (0)
+
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
 
 #ifndef PROT_READ
-- 
2.11.0

[PATCH 0/5] introduce SWAP macro

[René Scharfe]

Exchanging the value of two variables requires declaring a temporary
variable and repeating their names.  The swap macro in apply.c
simplifies this for its callers without changing the compiled binary.
Polish this macro and export it, then use it throughout the code to
reduce repetition and hide the declaration of temporary variables.

  add SWAP macro
  apply: use SWAP macro
  use SWAP macro
  diff: use SWAP macro
  graph: use SWAP macro

 apply.c   | 23 +++
 builtin/diff-tree.c   |  4 +---
 builtin/diff.c|  9 +++--
 contrib/coccinelle/swap.cocci | 28 
 diff-no-index.c   |  6 ++
 diff.c| 12 
 git-compat-util.h | 10 ++
 graph.c   | 11 ++-
 line-range.c  |  3 +--
 merge-recursive.c |  5 +
 object.c  |  4 +---
 pack-revindex.c   |  5 +
 prio-queue.c  |  4 +---
 strbuf.h  |  4 +---
 14 files changed, 63 insertions(+), 65 deletions(-)
 create mode 100644 contrib/coccinelle/swap.cocci

-- 
2.11.0

Re: [PATCH 2/2] use absolute_pathdup()

[René Scharfe]

Hi Dscho,

Am 27.01.2017 um 11:21 schrieb Johannes Schindelin:

On Thu, 26 Jan 2017, René Scharfe wrote:

Apply the symantic patch for converting callers that duplicate the


s/symantic/semantic/


thank you!  I wonder where this came from.  And where my spellchecker 
went without as much as a farewell.  Reinstalled it now..


René

[PATCH 2/2] use absolute_pathdup()

[René Scharfe]

Apply the symantic patch for converting callers that duplicate the
result of absolute_path() to call absolute_pathdup() instead, which
avoids an extra string copy to a static buffer.

Signed-off-by: Rene Scharfe 
---
 builtin/clone.c | 4 ++--
 builtin/submodule--helper.c | 2 +-
 worktree.c  | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 5ef81927a6..3f63edbbf9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -170,7 +170,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 
strbuf_addstr(, repo);
raw = get_repo_path_1(, is_bundle);
-   canon = raw ? xstrdup(absolute_path(raw)) : NULL;
+   canon = raw ? absolute_pathdup(raw) : NULL;
strbuf_release();
return canon;
 }
@@ -894,7 +894,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
 
path = get_repo_path(repo_name, _bundle);
if (path)
-   repo = xstrdup(absolute_path(repo_name));
+   repo = absolute_pathdup(repo_name);
else if (!strchr(repo_name, ':'))
die(_("repository '%s' does not exist"), repo_name);
else
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 74614a951e..899dc334e3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -626,7 +626,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
   module_clone_options);
 
strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
-   sm_gitdir = xstrdup(absolute_path(sb.buf));
+   sm_gitdir = absolute_pathdup(sb.buf);
strbuf_reset();
 
if (!is_absolute_path(path)) {
diff --git a/worktree.c b/worktree.c
index 53b4771c04..d633761575 100644
--- a/worktree.c
+++ b/worktree.c
@@ -145,7 +145,7 @@ static struct worktree *get_linked_worktree(const char *id)
 
 static void mark_current_worktree(struct worktree **worktrees)
 {
-   char *git_dir = xstrdup(absolute_path(get_git_dir()));
+   char *git_dir = absolute_pathdup(get_git_dir());
int i;
 
for (i = 0; worktrees[i]; i++) {
-- 
2.11.0

PATCH 1/2] abspath: add absolute_pathdup()

[René Scharfe]

Add a function that returns a buffer containing the absolute path of its
argument and a semantic patch for its intended use.  It avoids an extra
string copy to a static buffer.

Signed-off-by: Rene Scharfe 
---
 abspath.c| 7 +++
 cache.h  | 1 +
 contrib/coccinelle/xstrdup_or_null.cocci | 6 ++
 3 files changed, 14 insertions(+)

diff --git a/abspath.c b/abspath.c
index fce40fddcc..2f0c26e0e2 100644
--- a/abspath.c
+++ b/abspath.c
@@ -239,6 +239,13 @@ const char *absolute_path(const char *path)
return sb.buf;
 }
 
+char *absolute_pathdup(const char *path)
+{
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_add_absolute_path(, path);
+   return strbuf_detach(, NULL);
+}
+
 /*
  * Unlike prefix_path, this should be used if the named file does
  * not have to interact with index entry; i.e. name of a random file
diff --git a/cache.h b/cache.h
index 00a029af36..d7b7a8cd7a 100644
--- a/cache.h
+++ b/cache.h
@@ -1069,6 +1069,7 @@ const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
 char *real_pathdup(const char *path);
 const char *absolute_path(const char *path);
+char *absolute_pathdup(const char *path);
 const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf 
*sb);
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);
diff --git a/contrib/coccinelle/xstrdup_or_null.cocci 
b/contrib/coccinelle/xstrdup_or_null.cocci
index 3fceef132b..8e05d1ca4b 100644
--- a/contrib/coccinelle/xstrdup_or_null.cocci
+++ b/contrib/coccinelle/xstrdup_or_null.cocci
@@ -5,3 +5,9 @@ expression V;
 - if (E)
 -V = xstrdup(E);
 + V = xstrdup_or_null(E);
+
+@@
+expression E;
+@@
+- xstrdup(absolute_path(E))
++ absolute_pathdup(E)
-- 
2.11.0

Re: [DEMO][PATCH v2 6/5] compat: add a qsort_s() implementation based on GNU's qsort_r(1)

[René Scharfe]

Am 24.01.2017 um 21:39 schrieb Jeff King:

On Tue, Jan 24, 2017 at 07:00:03PM +0100, René Scharfe wrote:


I do worry about having to support more implementations in the
future that have different function signature for the comparison
callbacks, which will make things ugly, but this addition alone
doesn't look too bad to me.


It is unfair of me to show a 5% speedup and then recommend to not
include it. ;-)  That difference won't be measurable in real use cases
and the patch is not necessary.  This patch is simple, but the overall
complexity (incl. #ifdefs etc.) will be lower without it.


I care less about squeezing out the last few percent performance and
more that somebody libc qsort_r() might offer some other improvement.
For instance, it could sort in-place to lower memory use for some cases,
or do some clever thing that gives more than a few percent in the real
world (something like TimSort).

I don't know to what degree we should care about that.


That's a good question.  Which workloads spend a significant amount of 
time in qsort/qsort_s?


We could track processor time spent and memory allocated in QSORT_S and 
the whole program and show a warning at the end if one of the two 
exceeded, say, 5% of the total, asking nicely to send it to our mailing 
list.  Would something like this be useful for other functions or 
metrics as well?  Would it be too impolite to use users as telemetry 
transports?


If we find such cases then we'd better fix them for all platforms, e.g. 
by importing timsort, no?


René

Re: [PATCH v2 0/5] string-list: make string_list_sort() reentrant

[René Scharfe]

Am 24.01.2017 um 00:54 schrieb Jeff King:

The speed looks like a reasonable outcome. I'm torn on the qsort_r()
demo patch. I don't think it looks too bad. OTOH, I don't think I would
want to deal with the opposite-argument-order versions.


The code itself may look OK, but it's not really necessary and the 
special implementation for Linux makes increases maintenance costs.  Can 
we save it for later and first give the common implemention a chance to 
prove itself?



Is there any interest in people adding the ISO qsort_s() to their libc
implementations? It seems like it's been a fair number of years by now.


https://sourceware.org/ml/libc-alpha/2014-12/msg00513.html is the last 
post mentioning qsort_s on the glibc mailing list, but it didn't even 
make it into https://sourceware.org/glibc/wiki/Development_Todo/Master.
Not sure what's planned in BSD land, didn't find anything (but didn't 
look too hard).


René

Re: [DEMO][PATCH v2 6/5] compat: add a qsort_s() implementation based on GNU's qsort_r(1)

[René Scharfe]

Am 23.01.2017 um 20:07 schrieb Junio C Hamano:
> René Scharfe <l@web.de> writes:
> 
>> Implement qsort_s() as a wrapper to the GNU version of qsort_r(1) and
>> use it on Linux.  Performance increases slightly:
>>
>> Test HEAD^ HEAD
>> 
>> 0071.2: sort(1)  0.10(0.20+0.02)   0.10(0.21+0.01) +0.0%
>> 0071.3: string_list_sort()   0.17(0.15+0.01)   0.16(0.15+0.01) -5.9%
>>
>> Additionally the unstripped size of compat/qsort_s.o falls from 24576
>> to 16544 bytes in my build.
>>
>> IMHO these savings aren't worth the increased complexity of having to
>> support two implementations.
> 
> I do worry about having to support more implementations in the
> future that have different function signature for the comparison
> callbacks, which will make things ugly, but this addition alone
> doesn't look too bad to me.

It is unfair of me to show a 5% speedup and then recommend to not
include it. ;-)  That difference won't be measurable in real use cases
and the patch is not necessary.  This patch is simple, but the overall
complexity (incl. #ifdefs etc.) will be lower without it.

But here's another one, with even higher performance and with an even
bigger recommendation to not include it! :)  It veers off into another
direction: Parallel execution.  It requires thread-safe comparison
functions, which might surprise callers.  The value 1000 for the minimum
number of items before threading kicks in is just a guess, not based on
measurements.  So it's quite raw -- and I'm not sure why it's still a
bit slower than sort(1).

Test HEAD^ HEAD
-
0071.2: sort(1)  0.10(0.18+0.03)   0.10(0.20+0.02) +0.0%
0071.3: string_list_sort()   0.17(0.14+0.02)   0.11(0.18+0.02) -35.3%

---
 compat/qsort_s.c | 76 ++--
 1 file changed, 58 insertions(+), 18 deletions(-)

diff --git a/compat/qsort_s.c b/compat/qsort_s.c
index 52d1f0a73d..1304a089af 100644
--- a/compat/qsort_s.c
+++ b/compat/qsort_s.c
@@ -1,4 +1,5 @@
 #include "../git-compat-util.h"
+#include "../thread-utils.h"
 
 /*
  * A merge sort implementation, simplified from the qsort implementation
@@ -6,29 +7,58 @@
  * Added context pointer, safety checks and return value.
  */
 
-static void msort_with_tmp(void *b, size_t n, size_t s,
-  int (*cmp)(const void *, const void *, void *),
-  char *t, void *ctx)
+#define MIN_ITEMS_FOR_THREAD 1000
+
+struct work {
+   int nr_threads;
+   void *base;
+   size_t nmemb;
+   size_t size;
+   char *tmp;
+   int (*cmp)(const void *, const void *, void *);
+   void *ctx;
+};
+
+static void *msort_with_tmp(void *work)
 {
+   struct work one, two, *w = work;
char *tmp;
char *b1, *b2;
size_t n1, n2;
+   size_t s, n;
 
-   if (n <= 1)
-   return;
+   if (w->nmemb <= 1)
+   return NULL;
 
-   n1 = n / 2;
-   n2 = n - n1;
-   b1 = b;
-   b2 = (char *)b + (n1 * s);
+   one = two = *w;
+   one.nr_threads /= 2;
+   two.nr_threads -= one.nr_threads;
+   n = one.nmemb;
+   s = one.size;
+   n1 = one.nmemb = n / 2;
+   n2 = two.nmemb = n - n1;
+   b1 = one.base;
+   b2 = two.base = b1 + n1 * s;
+   two.tmp += n1 * s;
 
-   msort_with_tmp(b1, n1, s, cmp, t, ctx);
-   msort_with_tmp(b2, n2, s, cmp, t, ctx);
+#ifndef NO_PTHREADS
+   if (one.nr_threads && n > MIN_ITEMS_FOR_THREAD) {
+   pthread_t thread;
+   int err = pthread_create(, NULL, msort_with_tmp, );
+   msort_with_tmp();
+   if (err || pthread_join(thread, NULL))
+   msort_with_tmp();
+   } else
+#endif
+   {
+   msort_with_tmp();
+   msort_with_tmp();
+   }
 
-   tmp = t;
+   tmp = one.tmp;
 
while (n1 > 0 && n2 > 0) {
-   if (cmp(b1, b2, ctx) <= 0) {
+   if (one.cmp(b1, b2, one.ctx) <= 0) {
memcpy(tmp, b1, s);
tmp += s;
b1 += s;
@@ -42,7 +72,8 @@ static void msort_with_tmp(void *b, size_t n, size_t s,
}
if (n1 > 0)
memcpy(tmp, b1, n1 * s);
-   memcpy(b, t, (n - n2) * s);
+   memcpy(one.base, one.tmp, (n - n2) * s);
+   return NULL;
 }
 
 int git_qsort_s(void *b, size_t n, size_t s,
@@ -50,20 +81,29 @@ int git_qsort_s(void *b, size_t n, size_t s,
 {
const size_t size = st_mult(n, s);
char buf[1024];
+   struct work w;
 
if (!n)
return 0;
   

[DEMO][PATCH v2 6/5] compat: add a qsort_s() implementation based on GNU's qsort_r(1)

[René Scharfe]

Implement qsort_s() as a wrapper to the GNU version of qsort_r(1) and
use it on Linux.  Performance increases slightly:

Test HEAD^ HEAD

0071.2: sort(1)  0.10(0.20+0.02)   0.10(0.21+0.01) +0.0%
0071.3: string_list_sort()   0.17(0.15+0.01)   0.16(0.15+0.01) -5.9%

Additionally the unstripped size of compat/qsort_s.o falls from 24576
to 16544 bytes in my build.

IMHO these savings aren't worth the increased complexity of having to
support two implementations.
---
 Makefile |  6 ++
 compat/qsort_s.c | 18 ++
 config.mak.uname |  1 +
 3 files changed, 25 insertions(+)

diff --git a/Makefile b/Makefile
index 53ecc84e28..46db1c773f 100644
--- a/Makefile
+++ b/Makefile
@@ -282,6 +282,9 @@ all::
 # Define HAVE_ISO_QSORT_S if your platform provides a qsort_s() that's
 # compatible with the one described in C11 Annex K.
 #
+# Define HAVE_GNU_QSORT_R if your platform provides a qsort_r() that's
+# compatible with the one introduced with glibc 2.8.
+#
 # Define UNRELIABLE_FSTAT if your system's fstat does not return the same
 # information on a not yet closed file that lstat would return for the same
 # file after it was closed.
@@ -1426,6 +1429,9 @@ ifdef HAVE_ISO_QSORT_S
 else
COMPAT_OBJS += compat/qsort_s.o
 endif
+ifdef HAVE_GNU_QSORT_R
+   COMPAT_CFLAGS += -DHAVE_GNU_QSORT_R
+endif
 ifdef RUNTIME_PREFIX
COMPAT_CFLAGS += -DRUNTIME_PREFIX
 endif
diff --git a/compat/qsort_s.c b/compat/qsort_s.c
index 52d1f0a73d..763ee1faae 100644
--- a/compat/qsort_s.c
+++ b/compat/qsort_s.c
@@ -1,5 +1,21 @@
 #include "../git-compat-util.h"
 
+#if defined HAVE_GNU_QSORT_R
+
+int git_qsort_s(void *b, size_t n, size_t s,
+   int (*cmp)(const void *, const void *, void *), void *ctx)
+{
+   if (!n)
+   return 0;
+   if (!b || !cmp)
+   return -1;
+
+   qsort_r(b, n, s, cmp, ctx);
+   return 0;
+}
+
+#else
+
 /*
  * A merge sort implementation, simplified from the qsort implementation
  * by Mike Haertel, which is a part of the GNU C Library.
@@ -67,3 +83,5 @@ int git_qsort_s(void *b, size_t n, size_t s,
}
return 0;
 }
+
+#endif
diff --git a/config.mak.uname b/config.mak.uname
index 447f36ac2e..a1858f54ff 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -37,6 +37,7 @@ ifeq ($(uname_S),Linux)
NEEDS_LIBRT = YesPlease
HAVE_GETDELIM = YesPlease
SANE_TEXT_GREP=-a
+   HAVE_GNU_QSORT_R = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
HAVE_ALLOCA_H = YesPlease
-- 
2.11.0

[PATCH v2 2/5] add QSORT_S

[René Scharfe]

Add the macro QSORT_S, a convenient wrapper for qsort_s() that infers
the size of the array elements and dies on error.

Basically all possible errors are programming mistakes (passing NULL as
base of a non-empty array, passing NULL as comparison function,
out-of-bounds accesses), so terminating the program should be acceptable
for most callers.

Signed-off-by: Rene Scharfe 
---
 git-compat-util.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index f706744e6a..f46d40e4a4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -994,6 +994,11 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,
 #define qsort_s git_qsort_s
 #endif
 
+#define QSORT_S(base, n, compar, ctx) do { \
+   if (qsort_s((base), (n), sizeof(*(base)), compar, ctx)) \
+   die("BUG: qsort_s() failed");   \
+} while (0)
+
 #ifndef REG_STARTEND
 #error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
 #endif
-- 
2.11.0

[PATCH v2 5/5] ref-filter: use QSORT_S in ref_array_sort()

[René Scharfe]

Pass the array of sort keys to compare_refs() via the context parameter
of qsort_s() instead of using a global variable; that's cleaner and
simpler.  If ref_array_sort() is to be called from multiple parallel
threads then care still needs to be taken that the global variable
used_atom is not modified concurrently.

Signed-off-by: Rene Scharfe 
---
 ref-filter.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1a978405e6..3975022c88 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1589,8 +1589,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct 
ref_array_item *a, stru
return (s->reverse) ? -cmp : cmp;
 }
 
-static struct ref_sorting *ref_sorting;
-static int compare_refs(const void *a_, const void *b_)
+static int compare_refs(const void *a_, const void *b_, void *ref_sorting)
 {
struct ref_array_item *a = *((struct ref_array_item **)a_);
struct ref_array_item *b = *((struct ref_array_item **)b_);
@@ -1606,8 +1605,7 @@ static int compare_refs(const void *a_, const void *b_)
 
 void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
 {
-   ref_sorting = sorting;
-   QSORT(array->items, array->nr, compare_refs);
+   QSORT_S(array->items, array->nr, compare_refs, sorting);
 }
 
 static void append_literal(const char *cp, const char *ep, struct 
ref_formatting_state *state)
-- 
2.11.0

[PATCH v2 4/5] string-list: use QSORT_S in string_list_sort()

[René Scharfe]

Pass the comparison function to cmp_items() via the context parameter of
qsort_s() instead of using a global variable.  That allows calling
string_list_sort() from multiple parallel threads.

Our qsort_s() in compat/ is slightly slower than qsort(1) from glibc
2.24 for sorting lots of lines:

Test HEAD^ HEAD
-
0071.2: sort(1)  0.10(0.22+0.01)   0.09(0.21+0.00) -10.0%
0071.3: string_list_sort()   0.16(0.15+0.01)   0.17(0.15+0.00) +6.3%

GNU sort(1) version 8.26 is significantly faster because it uses
multiple parallel threads; with the unportable option --parallel=1 it
becomes slower:

Test HEAD^ HEAD

0071.2: sort(1)  0.21(0.18+0.01)   0.20(0.18+0.01) -4.8%
0071.3: string_list_sort()   0.16(0.13+0.02)   0.17(0.15+0.01) +6.3%

There is some instability -- the numbers for the sort(1) check shouldn't
be affected by this patch.  Anyway, the performance of our qsort_s()
implementation is apparently good enough, at least for this test.

Signed-off-by: Rene Scharfe 
---
 string-list.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/string-list.c b/string-list.c
index 8c83cac189..45016ad86d 100644
--- a/string-list.c
+++ b/string-list.c
@@ -211,21 +211,18 @@ struct string_list_item *string_list_append(struct 
string_list *list,
list->strdup_strings ? xstrdup(string) : (char 
*)string);
 }
 
-/* Yuck */
-static compare_strings_fn compare_for_qsort;
-
-/* Only call this from inside string_list_sort! */
-static int cmp_items(const void *a, const void *b)
+static int cmp_items(const void *a, const void *b, void *ctx)
 {
+   compare_strings_fn cmp = ctx;
const struct string_list_item *one = a;
const struct string_list_item *two = b;
-   return compare_for_qsort(one->string, two->string);
+   return cmp(one->string, two->string);
 }
 
 void string_list_sort(struct string_list *list)
 {
-   compare_for_qsort = list->cmp ? list->cmp : strcmp;
-   QSORT(list->items, list->nr, cmp_items);
+   QSORT_S(list->items, list->nr, cmp_items,
+   list->cmp ? list->cmp : strcmp);
 }
 
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
-- 
2.11.0

[PATCH v2 3/5] perf: add basic sort performance test

[René Scharfe]

Add a sort command to test-string-list that reads lines from stdin,
stores them in a string_list and then sorts it.  Use it in a simple
perf test script to measure the performance of string_list_sort().

Signed-off-by: Rene Scharfe 
---
 t/helper/test-string-list.c | 25 +
 t/perf/p0071-sort.sh| 26 ++
 2 files changed, 51 insertions(+)
 create mode 100755 t/perf/p0071-sort.sh

diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c
index 4a68967bd1..c502fa16d3 100644
--- a/t/helper/test-string-list.c
+++ b/t/helper/test-string-list.c
@@ -97,6 +97,31 @@ int cmd_main(int argc, const char **argv)
return 0;
}
 
+   if (argc == 2 && !strcmp(argv[1], "sort")) {
+   struct string_list list = STRING_LIST_INIT_NODUP;
+   struct strbuf sb = STRBUF_INIT;
+   struct string_list_item *item;
+
+   strbuf_read(, 0, 0);
+
+   /*
+* Split by newline, but don't create a string_list item
+* for the empty string after the last separator.
+*/
+   if (sb.buf[sb.len - 1] == '\n')
+   strbuf_setlen(, sb.len - 1);
+   string_list_split_in_place(, sb.buf, '\n', -1);
+
+   string_list_sort();
+
+   for_each_string_list_item(item, )
+   puts(item->string);
+
+   string_list_clear(, 0);
+   strbuf_release();
+   return 0;
+   }
+
fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
argv[1] ? argv[1] : "(there was none)");
return 1;
diff --git a/t/perf/p0071-sort.sh b/t/perf/p0071-sort.sh
new file mode 100755
index 00..7c9a35a646
--- /dev/null
+++ b/t/perf/p0071-sort.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+test_description='Basic sort performance tests'
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+test_expect_success 'setup' '
+   git ls-files --stage "*.[ch]" "*.sh" |
+   cut -f2 -d" " |
+   git cat-file --batch >unsorted
+'
+
+test_perf 'sort(1)' '
+   sort expect
+'
+
+test_perf 'string_list_sort()' '
+   test-string-list sort actual
+'
+
+test_expect_success 'string_list_sort() sorts like sort(1)' '
+   test_cmp_bin expect actual
+'
+
+test_done
-- 
2.11.0

[PATCH v2 1/5] compat: add qsort_s()

[René Scharfe]

The function qsort_s() was introduced with C11 Annex K; it provides the
ability to pass a context pointer to the comparison function, supports
the convention of using a NULL pointer for an empty array and performs a
few safety checks.

Add an implementation based on compat/qsort.c for platforms that lack a
native standards-compliant qsort_s() (i.e. basically everyone).  It
doesn't perform the full range of possible checks: It uses size_t
instead of rsize_t and doesn't check nmemb and size against RSIZE_MAX
because we probably don't have the restricted size type defined.  For
the same reason it returns int instead of errno_t.

Signed-off-by: Rene Scharfe 
---
 Makefile  |  8 +++
 compat/qsort_s.c  | 69 +++
 git-compat-util.h |  6 +
 3 files changed, 83 insertions(+)
 create mode 100644 compat/qsort_s.c

diff --git a/Makefile b/Makefile
index 27afd0f378..53ecc84e28 100644
--- a/Makefile
+++ b/Makefile
@@ -279,6 +279,9 @@ all::
 # is a simplified version of the merge sort used in glibc. This is
 # recommended if Git triggers O(n^2) behavior in your platform's qsort().
 #
+# Define HAVE_ISO_QSORT_S if your platform provides a qsort_s() that's
+# compatible with the one described in C11 Annex K.
+#
 # Define UNRELIABLE_FSTAT if your system's fstat does not return the same
 # information on a not yet closed file that lstat would return for the same
 # file after it was closed.
@@ -1418,6 +1421,11 @@ ifdef INTERNAL_QSORT
COMPAT_CFLAGS += -DINTERNAL_QSORT
COMPAT_OBJS += compat/qsort.o
 endif
+ifdef HAVE_ISO_QSORT_S
+   COMPAT_CFLAGS += -DHAVE_ISO_QSORT_S
+else
+   COMPAT_OBJS += compat/qsort_s.o
+endif
 ifdef RUNTIME_PREFIX
COMPAT_CFLAGS += -DRUNTIME_PREFIX
 endif
diff --git a/compat/qsort_s.c b/compat/qsort_s.c
new file mode 100644
index 00..52d1f0a73d
--- /dev/null
+++ b/compat/qsort_s.c
@@ -0,0 +1,69 @@
+#include "../git-compat-util.h"
+
+/*
+ * A merge sort implementation, simplified from the qsort implementation
+ * by Mike Haertel, which is a part of the GNU C Library.
+ * Added context pointer, safety checks and return value.
+ */
+
+static void msort_with_tmp(void *b, size_t n, size_t s,
+  int (*cmp)(const void *, const void *, void *),
+  char *t, void *ctx)
+{
+   char *tmp;
+   char *b1, *b2;
+   size_t n1, n2;
+
+   if (n <= 1)
+   return;
+
+   n1 = n / 2;
+   n2 = n - n1;
+   b1 = b;
+   b2 = (char *)b + (n1 * s);
+
+   msort_with_tmp(b1, n1, s, cmp, t, ctx);
+   msort_with_tmp(b2, n2, s, cmp, t, ctx);
+
+   tmp = t;
+
+   while (n1 > 0 && n2 > 0) {
+   if (cmp(b1, b2, ctx) <= 0) {
+   memcpy(tmp, b1, s);
+   tmp += s;
+   b1 += s;
+   --n1;
+   } else {
+   memcpy(tmp, b2, s);
+   tmp += s;
+   b2 += s;
+   --n2;
+   }
+   }
+   if (n1 > 0)
+   memcpy(tmp, b1, n1 * s);
+   memcpy(b, t, (n - n2) * s);
+}
+
+int git_qsort_s(void *b, size_t n, size_t s,
+   int (*cmp)(const void *, const void *, void *), void *ctx)
+{
+   const size_t size = st_mult(n, s);
+   char buf[1024];
+
+   if (!n)
+   return 0;
+   if (!b || !cmp)
+   return -1;
+
+   if (size < sizeof(buf)) {
+   /* The temporary array fits on the small on-stack buffer. */
+   msort_with_tmp(b, n, s, cmp, buf, ctx);
+   } else {
+   /* It's somewhat large, so malloc it.  */
+   char *tmp = xmalloc(size);
+   msort_with_tmp(b, n, s, cmp, tmp, ctx);
+   free(tmp);
+   }
+   return 0;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092b..f706744e6a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -988,6 +988,12 @@ static inline void sane_qsort(void *base, size_t nmemb, 
size_t size,
qsort(base, nmemb, size, compar);
 }
 
+#ifndef HAVE_ISO_QSORT_S
+int git_qsort_s(void *base, size_t nmemb, size_t size,
+   int (*compar)(const void *, const void *, void *), void *ctx);
+#define qsort_s git_qsort_s
+#endif
+
 #ifndef REG_STARTEND
 #error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
 #endif
-- 
2.11.0

[PATCH v2 0/5] string-list: make string_list_sort() reentrant

[René Scharfe]

Use qsort_s() from C11 Annex K to make string_list_sort() safer, in
particular when called from parallel threads.

Changes from v1:
* Renamed HAVE_QSORT_S to HAVE_ISO_QSORT_S in Makefile to disambiguate.
* Added basic perf test (patch 3).
* Converted a second caller to QSORT_S, in ref-filter.c (patch 5).

  compat: add qsort_s()
  add QSORT_S
  perf: add basic sort performance test
  string-list: use QSORT_S in string_list_sort()
  ref-filter: use QSORT_S in ref_array_sort()

 Makefile|  8 ++
 compat/qsort_s.c| 69 +
 git-compat-util.h   | 11 
 ref-filter.c|  6 ++--
 string-list.c   | 13 -
 t/helper/test-string-list.c | 25 
 t/perf/p0071-sort.sh| 26 +
 7 files changed, 146 insertions(+), 12 deletions(-)
 create mode 100644 compat/qsort_s.c
 create mode 100755 t/perf/p0071-sort.sh

-- 
2.11.0

Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context

[René Scharfe]

Am 15.01.2017 um 11:06 schrieb Vegard Nossum:

On 15/01/2017 03:39, Junio C Hamano wrote:

René Scharfe <l@web.de> writes:

How about extending the context upward only up to and excluding a line
that is either empty *or* a function line?  That would limit the extra
context to a single function in the worst case.

Reducing context at the bottom with the aim to remove comments for the
next section is more tricky as it could remove part of the function
that we'd like to show if we get the boundary wrong.  How bad would it
be to keep the southern border unchanged?


I personally do not think there is any robust heuristic other than
Vegard's "a blank line may be a signal enough that lines before that
are not part of the beginning of the function", and I think your
"hence we look for a blank line but if there is a line that matches
the function header, stop there as we know we came too far back"
will be a good-enough safety measure.

I also agree with you that we probably do not want to futz with the
southern border.


You are right, trying to change the southern border in this way is not
quite reliable if there are no empty lines whatsoever and can
erroneously cause the function context to not include the bottom of the
function being changed.

I'm splitting the function boundary detection logic into separate
functions and trying to solve the above case without breaking the tests
(and adding a new test for the above case too).

I'll see if I can additionally provide some toggles (flags or config
variables) to control the new behaviour, what I had in mind was:

  -W[=preamble,=no-preamble]
  --function-context[=preamble,=no-preamble]
  diff.functionContextPreamble = 

(where the new logic is controlled by the new config variable and
overridden by the presence of =preamble or =no-preamble).


Adding comments before a function is useful, removing comments after a 
function sounds to me as only nice to have (under the assumption that 
they belong to the next function[*]).  How bad would it be to only 
implement the first part (as in the patch I just sent) without adding 
new config settings or parameters?


Thanks,
René


[*] Silly counter-example (the #endif line):
#ifdef SOMETHING
int f(...) {
// implementation for SOMETHING
}
#else
inf f(...) {
// implementation without SOMETHING
}
#endif /* SOMETHING */

Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context

[René Scharfe]

Am 15.01.2017 um 03:39 schrieb Junio C Hamano:
> René Scharfe <l@web.de> writes:
> 
>>> I am also more focused on keeping the codebase maintainable in good
>>> health by making sure that we made an effort to find a solution that
>>> is general-enough before solving a single specific problem you have
>>> today.  We may end up deciding that a blank-line heuristics gives us
>>> good enough tradeoff, but I do not want us to make a decision before
>>> thinking.
>>
>> How about extending the context upward only up to and excluding a line
>> that is either empty *or* a function line?  That would limit the extra
>> context to a single function in the worst case.
>>
>> Reducing context at the bottom with the aim to remove comments for the
>> next section is more tricky as it could remove part of the function
>> that we'd like to show if we get the boundary wrong.  How bad would it
>> be to keep the southern border unchanged?
> 
> I personally do not think there is any robust heuristic other than
> Vegard's "a blank line may be a signal enough that lines before that
> are not part of the beginning of the function", and I think your
> "hence we look for a blank line but if there is a line that matches
> the function header, stop there as we know we came too far back"
> will be a good-enough safety measure.
> 
> I also agree with you that we probably do not want to futz with the
> southern border.

A replacement patch for 2/3 with these changes would look like this:

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 8c88dbde38..9ed54cd318 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -174,11 +174,11 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, 
xdemitcb_t *ecb,
s2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);
 
if (xecfg->flags & XDL_EMIT_FUNCCONTEXT) {
+   char dummy[1];
long fs1, i1 = xch->i1;
 
/* Appended chunk? */
if (i1 >= xe->xdf1.nrec) {
-   char dummy[1];
long i2 = xch->i2;
 
/*
@@ -200,6 +200,10 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, 
xdemitcb_t *ecb,
}
 
fs1 = get_func_line(xe, xecfg, NULL, i1, -1);
+   while (fs1 > 0 && !is_empty_rec(>xdf1, fs1 - 1) &&
+  match_func_rec(>xdf1, xecfg, fs1 - 1,
+ dummy, sizeof(dummy)) < 0)
+   fs1--;
if (fs1 < 0)
fs1 = 0;
if (fs1 < s1) {

Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,

[René Scharfe]

Am 13.01.2017 um 18:58 schrieb Elia Pinto:
> In this patch, instead of using xnprintf instead of snprintf, which asserts
> that we don't truncate, we are switching to dynamic allocation with  
> xstrfmt(),
> , so we can avoid dealing with magic numbers in the code and reduce the 
> cognitive burden from
> the programmers, because they no longer have to count bytes needed for static 
> allocation.
> As a side effect of this patch we have also reduced the snprintf() calls, 
> that may silently truncate 
> results if the programmer is not careful.
> 
> Helped-by: Junio C Hamano 
> Helped-by: Jeff King  
> Signed-off-by: Elia Pinto 
> ---
> This is the third  version of the patch.
> 
> Changes from the first version: I have split the original commit in two, as 
> discussed here
> http://public-inbox.org/git/20161213132717.42965-1-gitter.spi...@gmail.com/.
> 
> Changes from the second version:
> - Changed the commit message to clarify the purpose of the patch (
> as suggested by Junio)
> https://public-inbox.org/git/xmqqtw95mfo3@gitster.mtv.corp.google.com/T/#m2e6405a8a78a8ca1ed770614c91398290574c4a1
> 
> 
> 
>  builtin/commit.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 09bcc0f13..37228330c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1526,12 +1526,10 @@ static int git_commit_config(const char *k, const 
> char *v, void *cb)
>  static int run_rewrite_hook(const unsigned char *oldsha1,
>   const unsigned char *newsha1)
>  {
> - /* oldsha1 SP newsha1 LF NUL */
> - static char buf[2*40 + 3];
> + char *buf;
>   struct child_process proc = CHILD_PROCESS_INIT;
>   const char *argv[3];
>   int code;
> - size_t n;
>  
>   argv[0] = find_hook("post-rewrite");
>   if (!argv[0])
> @@ -1547,11 +1545,11 @@ static int run_rewrite_hook(const unsigned char 
> *oldsha1,
>   code = start_command();
>   if (code)
>   return code;
> - n = snprintf(buf, sizeof(buf), "%s %s\n",
> -  sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> + buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>   sigchain_push(SIGPIPE, SIG_IGN);
> - write_in_full(proc.in, buf, n);
> + write_in_full(proc.in, buf, strlen(buf));
>   close(proc.in);
> + free(buf);
>   sigchain_pop(SIGPIPE);
>   return finish_command();
>  }
> 

Perhaps I missed it from the discussion, but why not use strbuf?  It
would avoid counting the generated string's length.  That's probably
not going to make a measurable difference performance-wise, but it's
easy to avoid and doesn't even take up more lines:
---
 builtin/commit.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 711f96cc43..73bb72016f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1525,12 +1525,10 @@ static int git_commit_config(const char *k, const char 
*v, void *cb)
 static int run_rewrite_hook(const unsigned char *oldsha1,
const unsigned char *newsha1)
 {
-   /* oldsha1 SP newsha1 LF NUL */
-   static char buf[2*40 + 3];
+   struct strbuf sb = STRBUF_INIT;
struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
int code;
-   size_t n;
 
argv[0] = find_hook("post-rewrite");
if (!argv[0])
@@ -1546,11 +1544,11 @@ static int run_rewrite_hook(const unsigned char 
*oldsha1,
code = start_command();
if (code)
return code;
-   n = snprintf(buf, sizeof(buf), "%s %s\n",
-sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+   strbuf_addf(, "%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
sigchain_push(SIGPIPE, SIG_IGN);
-   write_in_full(proc.in, buf, n);
+   write_in_full(proc.in, sb.buf, sb.len);
close(proc.in);
+   strbuf_release();
sigchain_pop(SIGPIPE);
return finish_command();
 }
-- 
2.11.0

Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context

[René Scharfe]

Am 14.01.2017 um 00:56 schrieb Junio C Hamano:

Vegard Nossum  writes:


The patch will work as intended and as expected for 95% of the users out
there (javadoc, Doxygen, kerneldoc, etc. all have the comment
immediately preceding the function) and fixes a very real problem for me
(and I expect many others) _today_; for the remaining 5% (who put a
blank line between their comment and the start of the function) it will
revert back to the current behaviour, so there should be no regression
for them.


I notice your 95% are all programming languages, but I am more
worried about the contents written in non programming languages
(René gave HTML an an example--there may be other types of contents
that we programmer types do not deal with every day, but Git users
depend on).

I am also more focused on keeping the codebase maintainable in good
health by making sure that we made an effort to find a solution that
is general-enough before solving a single specific problem you have
today.  We may end up deciding that a blank-line heuristics gives us
good enough tradeoff, but I do not want us to make a decision before
thinking.


How about extending the context upward only up to and excluding a line 
that is either empty *or* a function line?  That would limit the extra 
context to a single function in the worst case.


Reducing context at the bottom with the aim to remove comments for the 
next section is more tricky as it could remove part of the function that 
we'd like to show if we get the boundary wrong.  How bad would it be to 
keep the southern border unchanged?


René

Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context

[René Scharfe]

Am 13.01.2017 um 17:15 schrieb Vegard Nossum:

When using -W to include the whole function in the diff context, you
are typically doing this to be able to review the change in its entirety
within the context of the function. It is therefore almost always
desirable to include any comments that immediately precede the function.

This also the fixes the case for C where the declaration is split across
multiple lines (where the first line of the declaration would not be
included in the output), e.g.:

void
dummy(void)
{
...
}



That's true, but I'm not sure "non-empty line before function line" is 
good enough a definition for desirable lines.  It wouldn't work for 
people who don't believe in empty lines.  Or for those that put a blank 
line between comment and function.  (I have an opinion on such habits, 
but git diff should probably stay neutral.)  And that's just for C code; 
I have no idea how this heuristic would hold up for other file types 
like HTML.


We can identify function lines with arbitrary precision (with a 
xfuncname regex, if needed), but there is no accurate way to classify 
lines as comments, or as the end of functions.  Adding optional regexes 
for single- and multi-line comments would help, at least for C.


René

Re: [PATCH 1/3] xdiff: -W: relax end-of-file function detection

[René Scharfe]

Am 13.01.2017 um 17:15 schrieb Vegard Nossum:

When adding a new function to the end of a file, it's enough to know
that 1) the addition is at the end of the file; and 2) there is a
function _somewhere_ in there.

If we had simply been changing the end of an existing function, then we
would also be deleting something from the old version.


That makes sense, thanks.


This fixes the case where we add e.g.

// Begin of dummy
static int dummy(void)
{
}

to the end of the file.


Without this patch the unchanged function before the added lines is 
shown in its entirety as (uncalled for) context.




Cc: René Scharfe <l@web.de>
Signed-off-by: Vegard Nossum <vegard.nos...@oracle.com>
---
 xdiff/xemit.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 7389ce4..8c88dbd 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -183,16 +183,14 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, 
xdemitcb_t *ecb,

/*
 * We don't need additional context if
-* a whole function was added, possibly
-* starting with empty lines.
+* a whole function was added.
 */
-   while (i2 < xe->xdf2.nrec &&
-  is_empty_rec(>xdf2, i2))
+   while (i2 < xe->xdf2.nrec) {
+   if (match_func_rec(>xdf2, xecfg, i2,
+   dummy, sizeof(dummy)) >= 0)


Nit: I don't like the indentation here.  Giving "dummy" its own line is 
also not exactly pretty, but at least would allow the parameters to be 
aligned on the opening parenthesis.



+   goto post_context_calculation;
i2++;
-   if (i2 < xe->xdf2.nrec &&
-   match_func_rec(>xdf2, xecfg, i2,
-  dummy, sizeof(dummy)) >= 0)
-   goto post_context_calculation;
+   }

/*
 * Otherwise get more context from the

Re: [PATCH] archive-zip: load userdiff config

[René Scharfe]

Am 02.01.2017 um 23:25 schrieb Jeff King:

Since 4aff646d17 (archive-zip: mark text files in archives,
2015-03-05), the zip archiver will look at the userdiff
driver to decide whether a file is text or binary. This
usually doesn't need to look any further than the attributes
themselves (e.g., "-diff", etc). But if the user defines a
custom driver like "diff=foo", we need to look at
"diff.foo.binary" in the config. Prior to this patch, we
didn't actually load it.


Ah, didn't think of that, obviously.

Would it make sense for userdiff_find_by_path() to die if 
userdiff_config() hasn't been called, yet?



I also happened to notice that zipfiles are created using the local
timezone (because they have no notion of the timezone, so we have to
pick _something_). That's probably the least-terrible option, but it was
certainly surprising to me when I tried to bit-for-bit reproduce a
zipfile from GitHub on my local machine.


That reminds me of an old request to allow users better control over the 
meta-data written into archives.  Being able to specify a time zone 
offset could be a start.



 archive-zip.c  |  7 +++
 t/t5003-archive-zip.sh | 22 ++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 9db47357b0..b429a8d974 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -554,11 +554,18 @@ static void dos_time(time_t *time, int *dos_date, int 
*dos_time)
*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
 }

+static int archive_zip_config(const char *var, const char *value, void *data)
+{
+   return userdiff_config(var, value);
+}
+
 static int write_zip_archive(const struct archiver *ar,
 struct archiver_args *args)
 {
int err;

+   git_config(archive_zip_config, NULL);
+


I briefly thought about moving this call to archive.c with the rest of 
the config-related stuff, but I agree it's better kept here.



dos_time(>time, _date, _time);

zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 14744b2a4b..55c7870997 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -64,6 +64,12 @@ check_zip() {
test_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&
test_cmp_bin $original/nodiff.lf   $extracted/nodiff.lf
"
+
+   test_expect_success UNZIP " validate that custom diff is unchanged " "
+   test_cmp_bin $original/custom.cr   $extracted/custom.cr &&
+   test_cmp_bin $original/custom.crlf $extracted/custom.crlf &&
+   test_cmp_bin $original/custom.lf   $extracted/custom.lf
+   "
 }

 test_expect_success \
@@ -78,6 +84,9 @@ test_expect_success \
  printf "text\r" >a/nodiff.cr &&
  printf "text\r\n"   >a/nodiff.crlf &&
  printf "text\n" >a/nodiff.lf &&
+ printf "text\r" >a/custom.cr &&
+ printf "text\r\n"   >a/custom.crlf &&
+ printf "text\n" >a/custom.lf &&
  printf "\0\r"   >a/binary.cr &&
  printf "\0\r\n" >a/binary.crlf &&
  printf "\0\n"   >a/binary.lf &&
@@ -112,15 +121,20 @@ test_expect_success 'add files to repository' '
 test_expect_success 'setup export-subst and diff attributes' '
echo "a/nodiff.* -diff" >>.git/info/attributes &&
echo "a/diff.* diff" >>.git/info/attributes &&
+   echo "a/custom.* diff=custom" >>.git/info/attributes &&
+   git config diff.custom.binary true &&
echo "substfile?" export-subst >>.git/info/attributes &&
git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
>a/substfile1
 '

-test_expect_success \
-'create bare clone' \
-'git clone --bare . bare.git &&
- cp .git/info/attributes bare.git/info/attributes'
+test_expect_success 'create bare clone' '
+   git clone --bare . bare.git &&
+   cp .git/info/attributes bare.git/info/attributes &&
+   # Recreate our changes to .git/config rather than just copying it, as
+   # we do not want to clobber core.bare or other settings.
+   git -C bare.git config diff.custom.binary true
+'

 test_expect_success \
 'remove ignored file' \



Looks good, thanks!

René

Re: [PATCH] submodule.c: use GIT_DIR_ENVIRONMENT consistently

[René Scharfe]

Am 30.12.2016 um 01:47 schrieb Stefan Beller:

diff --git a/submodule.c b/submodule.c
index ece17315d6..973b9f3f96 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1333,5 +1333,6 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
-   argv_array_push(out, "GIT_DIR=.git");
+   argv_array_push(out, "%s=%s", GIT_DIR_ENVIRONMENT,
+   DEFAULT_GIT_DIR_ENVIRONMENT);


argv_array_pushf (with added "f") instead?

And indent continued lines to align them with the left parenthesis, like 
this:


fn(arg1,
   arg2);


 }

Re: [PATCH] string-list: make compare function compatible with qsort(3)

[René Scharfe]

Am 21.12.2016 um 17:12 schrieb Jeff King:

On Wed, Dec 21, 2016 at 10:36:41AM +0100, René Scharfe wrote:


One shortcoming is that the comparison function is restricted to working
with the string members of items; util is inaccessible to it.  Another
one is that the value of cmp is passed in a global variable to
cmp_items(), making string_list_sort() non-reentrant.


I think this approach is OK for string_list, but it doesn't help the
general case that wants qsort_s() to actually access global data. I
don't know how common that is in our codebase, though.

So I'm fine with it, but I think we might eventually need to revisit the
qsort_s() thing anyway.


I have to admit I didn't even consider the possibility that the pattern 
of accessing global variables in qsort(1) compare functions could have 
spread.


And indeed, at least ref-filter.c::compare_refs() and 
worktree.c::compare_worktree() so that as well.  The latter uses 
fspathcmp(), which is OK as ignore_case is only set once when reading 
the config, but the first one looks, well, interesting.  Perhaps a 
single ref filter per program is enough?


Anyway, that's a good enough argument for me for adding that newfangled 
C11 function after all..


Btw.: Found with

  git grep 'QSORT.*;$' |
  sed 's/.* /int /; s/);//' |
  sort -u |
  git grep -Ww -f-

and

  git grep -A1 'QSORT.*,$'


Remove the intermediate layer, i.e. cmp_items(), make the comparison
functions compatible with qsort(3) and pass them pointers to full items.
This allows comparisons to also take the util member into account, and
avoids the need to pass the real comparison function to an intermediate
function, removing the need for a global function.


I'm not sure if access to the util field is really of any value, after
looking at it in:

  
http://public-inbox.org/git/20161125171546.fa3zpapbjngjc...@sigill.intra.peff.net/

Though note that if we do take this patch, there are probably one or two
spots that could switch from QSORT() to string_list_sort().


Yes, but as you noted in that thread there is not much point in doing 
that; the only net win is that we can pass a list as a single pointer 
instead of as base pointer and element count -- the special compare 
function needs to be specified anyway (once), somehow.


René

Re: [PATCH] unpack-trees: move checkout state into check_updates

[René Scharfe]

Am 29.12.2016 um 00:26 schrieb Stefan Beller:

The checkout state was introduced via 16da134b1f9
(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
checkout state explicitly to check_updates(), 2016-09-13), but we can
go even further.

The `struct checkout state` is not used in unpack_trees apart from
initializing it, so move it into the function that makes use of it,
which is `check_updates`.


Thanks for catching this, the result looks nicer.


Signed-off-by: Stefan Beller 
---
 unpack-trees.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index ea799d37c5..78703af135 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -224,14 +224,18 @@ static void unlink_entry(const struct cache_entry *ce)
schedule_dir_for_removal(ce->name, ce_namelen(ce));
 }

-static int check_updates(struct unpack_trees_options *o,
-const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
 {
unsigned cnt = 0, total = 0;
struct progress *progress = NULL;
struct index_state *index = >result;
int i;
int errs = 0;
+   struct checkout state = CHECKOUT_INIT;
+   state.force = 1;
+   state.quiet = 1;
+   state.refresh_cache = 1;
+   state.istate = >result;


And here (as well as in two more places in this function) we could use 
"index" instead of ">result".  Not sure if it's worth a patch, though.




if (o->update && o->verbose_update) {
for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
@@ -270,7 +274,7 @@ static int check_updates(struct unpack_trees_options *o,
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
-   errs |= checkout_entry(ce, state, NULL);
+   errs |= checkout_entry(ce, , NULL);
}
}
}
@@ -1100,14 +1104,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
int i, ret;
static struct cache_entry *dfc;
struct exclude_list el;
-   struct checkout state = CHECKOUT_INIT;

if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
-   state.force = 1;
-   state.quiet = 1;
-   state.refresh_cache = 1;
-   state.istate = >result;

memset(, 0, sizeof(el));
if (!core_apply_sparse_checkout || !o->update)
@@ -1244,7 +1243,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
}

o->src_index = NULL;
-   ret = check_updates(o, ) ? (-2) : 0;
+   ret = check_updates(o) ? (-2) : 0;
if (o->dst_index) {
if (!ret) {
if (!o->result.cache_tree)

[PATCH] string-list: make compare function compatible with qsort(3)

[René Scharfe]

The cmp member of struct string_list points to a comparison function
that is used for sorting and searching of list items.  It takes two
string pointers -- like strcmp(3), which is in fact the default;
cmp_items() provides a qsort(3) compatible interface by passing the
string members of two struct string_list_item pointers to cmp.

One shortcoming is that the comparison function is restricted to working
with the string members of items; util is inaccessible to it.  Another
one is that the value of cmp is passed in a global variable to
cmp_items(), making string_list_sort() non-reentrant.

Remove the intermediate layer, i.e. cmp_items(), make the comparison
functions compatible with qsort(3) and pass them pointers to full items.
This allows comparisons to also take the util member into account, and
avoids the need to pass the real comparison function to an intermediate
function, removing the need for a global function.

A downside is that comparison functions take void pointers now and each
of them needs to cast its arguments to struct string_list_item pointers,
weakening type safety and adding some repetitive code.  Programmers are
used to that, however, as that's par for the course with qsort(3).

Also two unsightly casts are added that remove the const qualifiers of
strings while building the structs to pass to the comparison function as
search keys.  It should be safe, though, as we only ever use them for
reading.

Signed-off-by: Rene Scharfe 
---
Alternative approach to the qsort_s()-based series "string-list: make
string_list_sort() reentrant".

 Documentation/technical/api-string-list.txt |  6 +++--
 builtin/mailsplit.c |  5 +++-
 mailmap.c   |  5 ++--
 merge-recursive.c   |  4 ++-
 string-list.c   | 39 +++--
 string-list.h   |  4 +--
 tmp-objdir.c|  4 ++-
 7 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/Documentation/technical/api-string-list.txt 
b/Documentation/technical/api-string-list.txt
index c08402b12..39eac59c7 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -205,5 +205,7 @@ Represents the list itself.
   You should not tamper with it.
 . Setting the `strdup_strings` member to 1 will strdup() the strings
   before adding them, see above.
-. The `compare_strings_fn` member is used to specify a custom compare
-  function, otherwise `strcmp()` is used as the default function.
+. The `cmp` member is used to specify a custom compare function. It has
+  the same signature as the one for qsort(1) and is passed two pointers
+  to `struct string_list_item`. If it's NULL then the `string` members
+  are compared with `strcmp(1)`; this is the default.
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c..4e72e3128 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -147,8 +147,11 @@ static int populate_maildir_list(struct string_list *list, 
const char *path)
return ret;
 }
 
-static int maildir_filename_cmp(const char *a, const char *b)
+static int maildir_filename_cmp(const void *one, const void *two)
 {
+   const struct string_list_item *item_one = one, *item_two = two;
+   const char *a = item_one->string, *b = item_two->string;
+
while (*a && *b) {
if (isdigit(*a) && isdigit(*b)) {
long int na, nb;
diff --git a/mailmap.c b/mailmap.c
index c1a79c100..5290b5153 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -61,9 +61,10 @@ static void free_mailmap_entry(void *p, const char *s)
  * namemap.cmp until we know no systems that matter have such an
  * "unusual" string.h.
  */
-static int namemap_cmp(const char *a, const char *b)
+static int namemap_cmp(const void *one, const void *two)
 {
-   return strcasecmp(a, b);
+   const struct string_list_item *item_one = one, *item_two = two;
+   return strcasecmp(item_one->string, item_two->string);
 }
 
 static void add_mapping(struct string_list *map,
diff --git a/merge-recursive.c b/merge-recursive.c
index d32720944..4683ba43f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -390,8 +390,10 @@ static struct string_list *get_unmerged(void)
return unmerged;
 }
 
-static int string_list_df_name_compare(const char *one, const char *two)
+static int string_list_df_name_compare(const void *a, const void *b)
 {
+   const struct string_list_item *item_a = a, *item_b = b;
+   const char *one = item_a->string, *two = item_b->string;
int onelen = strlen(one);
int twolen = strlen(two);
/*
diff --git a/string-list.c b/string-list.c
index 8c83cac18..c583a04ee 100644
--- a/string-list.c
+++ b/string-list.c
@@ -7,17 +7,26 @@ void string_list_init(struct string_list *list, int 
strdup_strings)
list->strdup_strings = strdup_strings;
 }
 
+static 

Re: [PATCH 1/3] compat: add qsort_s()

[René Scharfe]

Am 12.12.2016 um 20:57 schrieb Jeff King:

On Mon, Dec 12, 2016 at 08:51:14PM +0100, René Scharfe wrote:


It's kinda cool to have a bespoke compatibility layer for major platforms,
but the more I think about it the less I can answer why we would want that.
Safety, reliability and performance can't be good reasons -- if our fallback
function lacks in these regards then we have to improve it in any case.


There may be cases that we don't want to support because of portability
issues. E.g., if your libc has an assembly-optimized qsort() we wouldn't
want to replicate that.


Offloading to GPUs might be a better example; I don't know of a libc 
that does any of that, though (yet).



I dunno. I am not that opposed to just saying "forget libc qsort, we
always use our internal one which is consistent, performant, and safe".
But when I suggested something similar for our regex library, I seem to
recall there were complaints.


Well, I'm not sure how comparable they are, but perhaps we can avoid 
compat code altogether in this case.  Patch coming in a new thread.


René

Re: [PATCH 1/3] compat: add qsort_s()

[René Scharfe]

Am 01.12.2016 um 21:22 schrieb Junio C Hamano:

Junio C Hamano  writes:


Eh, wait.  BSD and Microsoft have paramters reordered in the
callback comparison function.  I suspect that would not fly very
well.


Hmm.  We could do it like this, which may not be too bad.


It's kinda cool to have a bespoke compatibility layer for major 
platforms, but the more I think about it the less I can answer why we 
would want that.  Safety, reliability and performance can't be good 
reasons -- if our fallback function lacks in these regards then we have 
to improve it in any case.


Text size could be a valid reason, but the full function only adds a bit 
more than 2KB to the unstripped git binary.


The flip side is we'd build an ifdef maze that's harder to read and a 
lot more difficult to test.


What do we get in return for that additional complexity?


#if APPLE_QSORT_R
struct apple_qsort_adapter {
int (*user_cmp)(const void *, const void *, void *);
void *user_ctx;
}

static int apple_qsort_adapter_cmp(void *ctx, const void *a, const void *b)
{
struct apple_qsort_adapter *wrapper_ctx = ctx;
return wrapper_ctx->user_cmp(a, b, wrapper_ctx->user_ctx);
}
#endif

int git_qsort_s(void *b, size_t n, size_t s,
   int (*cmp)(const void *, const void *, void *), void *ctx)
{
if (!n)
return 0;
if (!b || !cmp)
return -1;
#if GNU_QSORT_R
qsort_r(b, n, s, cmp, ctx);
#elif APPLE_QSORT_R
{
struct appple_qsort_adapter a = { cmp, ctx };
qsort_r(b, n, s, , appple_qsort_adapter_cmp);
}
#endif


Nit: The fallback for non-GNU, non-Apple systems is missing here, but 
the idea is illustrated clearly enough.



  return 0;
}

Re: [PATCH 1/3] compat: add qsort_s()

[René Scharfe]

Am 01.12.2016 um 21:19 schrieb Jeff King:

On Thu, Dec 01, 2016 at 12:14:42PM -0800, Junio C Hamano wrote:


Jeff King  writes:


To make matters more fun, apparently[1] there are multiple variants of
qsort_r with different argument orders. _And_ apparently Microsoft
defines qsort_s, but it's not quite the same thing. But all of that can
be dealt with by having more specific flags (HAVE_GNU_QSORT_R, etc).


AFAIU it went like this:

// FreeBSD 5.0 (2003)
void qsort_r(void *base, size_t nmemb, size_t size,
void *context,
int (*compar)(void *context, const void *x, const void *y));

// Microsoft Visual Studio 2005
void qsort_s(void *base, size_t nmemb, size_t size,
int (*compar)(void *context, const void *x, const void *y),
void *context);

// glibc 2.8 (2008)
void qsort_r(void *base, size_t nmemb, size_t size,
int (*compar)(const void *x, const void *y, void *context),
void *context);

// C11 Annex K (2011)
errno_t qsort_s(void *base, rsize_t nmemb, rsize_t size,
int (*compar)(const void *x, const void *y, void *context),
void *context);


It just seems like we should be able to do a better job of using the
system qsort in many cases.


Sure, platform-specific implementations can be shorter.


If we were to go that route, perhaps we shouldn't have HAVE_QSORT_S
so that Microsoft folks won't define it by mistake (instead perhaps
call it HAVE_ISO_QSORT_S or something).


OK.


I like your suggestion in general.  The body of git_qsort_s() on
systems without ISO_QSORT_S can do

 - GNU qsort_r() without any change in the parameters,

 - Microsoft qsort_s() with parameter reordered, or

 - Apple/BSD qsort_r() with parameter reordered.

and that would cover the major platforms.


Yes.

However, for MSys INTERNAL_QSORT is defined for some reason, so the 
platform's qsort(3) is not used there; I guess the same reason applies 
to qsort_s().  If it doesn't then an implementation may want to convert 
a call to the invalid parameter handler (which may show a dialog 
offering to Retry, Continue or Abort) into a non-zero return value.



Eh, wait.  BSD and Microsoft have paramters reordered in the
callback comparison function.  I suspect that would not fly very
well.


You can hack around it by passing a wrapper callback that flips the
arguments. Since we have a "void *" data pointer, that would point to a
struct holding the "real" callback and chaining to the original data
pointer.

It does incur the cost of an extra level of indirection for each
comparison, though (not just for each qsort call).


Indeed.  We'd need a perf test to measure that overhead before we could 
determine if that's a problem, though.



You could do it as zero-cost if you were willing to turn the comparison
function definition into a macro.


Ugh.  That either requires changing the signature of qsort_s() based on 
the underlying native function as well, or using a void pointer to pass 
the comparison function, no?  Let's not do that, at least not without a 
good reason.


René

Re: [PATCH 1/3] compat: add qsort_s()

[René Scharfe]

Am 01.12.2016 um 17:26 schrieb René Scharfe:
> The function qsort_s() was introduced with C11 Annex K; it provides the
> ability to pass a context pointer to the comparison function, supports
> the convention of using a NULL pointer for an empty array and performs a
> few safety checks.
> 
> Add an implementation based on compat/qsort.c for platforms that lack a
> native standards-compliant qsort_s() (i.e. basically everyone).  It
> doesn't perform the full range of possible checks: It uses size_t
> instead of rsize_t and doesn't check nmemb and size against RSIZE_MAX
> because we probably don't have the restricted size type defined.  For
> the same reason it returns int instead of errno_t.
> 
> Signed-off-by: Rene Scharfe <l@web.de>
> ---
>  Makefile  | 10 
>  compat/qsort_s.c  | 69 
> +++
>  git-compat-util.h |  6 +
>  3 files changed, 85 insertions(+)
>  create mode 100644 compat/qsort_s.c

And here's the diff for compat/qsort_s.c with copy detection (-C -C):
---
 compat/{qsort.c => qsort_s.c} | 25 -
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/compat/qsort.c b/compat/qsort_s.c
similarity index 62%
copy from compat/qsort.c
copy to compat/qsort_s.c
index 7d071afb7..52d1f0a73 100644
--- a/compat/qsort.c
+++ b/compat/qsort_s.c
@@ -3,11 +3,12 @@
 /*
  * A merge sort implementation, simplified from the qsort implementation
  * by Mike Haertel, which is a part of the GNU C Library.
+ * Added context pointer, safety checks and return value.
  */
 
 static void msort_with_tmp(void *b, size_t n, size_t s,
-  int (*cmp)(const void *, const void *),
-  char *t)
+  int (*cmp)(const void *, const void *, void *),
+  char *t, void *ctx)
 {
char *tmp;
char *b1, *b2;
@@ -21,13 +22,13 @@ static void msort_with_tmp(void *b, size_t n, size_t s,
b1 = b;
b2 = (char *)b + (n1 * s);
 
-   msort_with_tmp(b1, n1, s, cmp, t);
-   msort_with_tmp(b2, n2, s, cmp, t);
+   msort_with_tmp(b1, n1, s, cmp, t, ctx);
+   msort_with_tmp(b2, n2, s, cmp, t, ctx);
 
tmp = t;
 
while (n1 > 0 && n2 > 0) {
-   if (cmp(b1, b2) <= 0) {
+   if (cmp(b1, b2, ctx) <= 0) {
memcpy(tmp, b1, s);
tmp += s;
b1 += s;
@@ -44,19 +45,25 @@ static void msort_with_tmp(void *b, size_t n, size_t s,
memcpy(b, t, (n - n2) * s);
 }
 
-void git_qsort(void *b, size_t n, size_t s,
-  int (*cmp)(const void *, const void *))
+int git_qsort_s(void *b, size_t n, size_t s,
+   int (*cmp)(const void *, const void *, void *), void *ctx)
 {
const size_t size = st_mult(n, s);
char buf[1024];
 
+   if (!n)
+   return 0;
+   if (!b || !cmp)
+   return -1;
+
if (size < sizeof(buf)) {
/* The temporary array fits on the small on-stack buffer. */
-   msort_with_tmp(b, n, s, cmp, buf);
+   msort_with_tmp(b, n, s, cmp, buf, ctx);
} else {
/* It's somewhat large, so malloc it.  */
char *tmp = xmalloc(size);
-   msort_with_tmp(b, n, s, cmp, tmp);
+   msort_with_tmp(b, n, s, cmp, tmp, ctx);
free(tmp);
}
+   return 0;
 }

[PATCH 3/3] string-list: use QSORT_S in string_list_sort()

[René Scharfe]

Pass the comparison function to cmp_items() via the context parameter of
qsort_s() instead of using a global variable.  That allows calling
string_list_sort() from multiple parallel threads.

Signed-off-by: Rene Scharfe 
---
 string-list.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/string-list.c b/string-list.c
index 8c83cac18..45016ad86 100644
--- a/string-list.c
+++ b/string-list.c
@@ -211,21 +211,18 @@ struct string_list_item *string_list_append(struct 
string_list *list,
list->strdup_strings ? xstrdup(string) : (char 
*)string);
 }
 
-/* Yuck */
-static compare_strings_fn compare_for_qsort;
-
-/* Only call this from inside string_list_sort! */
-static int cmp_items(const void *a, const void *b)
+static int cmp_items(const void *a, const void *b, void *ctx)
 {
+   compare_strings_fn cmp = ctx;
const struct string_list_item *one = a;
const struct string_list_item *two = b;
-   return compare_for_qsort(one->string, two->string);
+   return cmp(one->string, two->string);
 }
 
 void string_list_sort(struct string_list *list)
 {
-   compare_for_qsort = list->cmp ? list->cmp : strcmp;
-   QSORT(list->items, list->nr, cmp_items);
+   QSORT_S(list->items, list->nr, cmp_items,
+   list->cmp ? list->cmp : strcmp);
 }
 
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
-- 
2.11.0

[PATCH 2/3] add QSORT_S

[René Scharfe]

Add the macro QSORT_S, a convenient wrapper for qsort_s() that infers
the size of the array elements and dies on error.

Basically all possible errors are programming mistakes (passing NULL as
base of a non-empty array, passing NULL as comparison function,
out-of-bounds accesses), so terminating the program should be acceptable
for most callers.

Signed-off-by: Rene Scharfe 
---
 git-compat-util.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index d25f0bd4c..b707dd880 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -994,6 +994,11 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,
 #define qsort_s git_qsort_s
 #endif
 
+#define QSORT_S(base, n, compar, ctx) do { \
+   if (qsort_s((base), (n), sizeof(*(base)), compar, ctx)) \
+   die("BUG: qsort_s() failed");   \
+} while (0)
+
 #ifndef REG_STARTEND
 #error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
 #endif
-- 
2.11.0

[PATCH 1/3] compat: add qsort_s()

[René Scharfe]

The function qsort_s() was introduced with C11 Annex K; it provides the
ability to pass a context pointer to the comparison function, supports
the convention of using a NULL pointer for an empty array and performs a
few safety checks.

Add an implementation based on compat/qsort.c for platforms that lack a
native standards-compliant qsort_s() (i.e. basically everyone).  It
doesn't perform the full range of possible checks: It uses size_t
instead of rsize_t and doesn't check nmemb and size against RSIZE_MAX
because we probably don't have the restricted size type defined.  For
the same reason it returns int instead of errno_t.

Signed-off-by: Rene Scharfe 
---
 Makefile  | 10 
 compat/qsort_s.c  | 69 +++
 git-compat-util.h |  6 +
 3 files changed, 85 insertions(+)
 create mode 100644 compat/qsort_s.c

diff --git a/Makefile b/Makefile
index f53fcc90d..2245fd95d 100644
--- a/Makefile
+++ b/Makefile
@@ -279,6 +279,9 @@ all::
 # is a simplified version of the merge sort used in glibc. This is
 # recommended if Git triggers O(n^2) behavior in your platform's qsort().
 #
+# Define HAVE_QSORT_S if your platform provides a qsort_s() that's
+# compatible with the one described in C11 Annex K.
+#
 # Define UNRELIABLE_FSTAT if your system's fstat does not return the same
 # information on a not yet closed file that lstat would return for the same
 # file after it was closed.
@@ -1423,6 +1426,13 @@ ifdef INTERNAL_QSORT
COMPAT_CFLAGS += -DINTERNAL_QSORT
COMPAT_OBJS += compat/qsort.o
 endif
+
+ifdef HAVE_QSORT_S
+   COMPAT_CFLAGS += -DHAVE_QSORT_S
+else
+   COMPAT_OBJS += compat/qsort_s.o
+endif
+
 ifdef RUNTIME_PREFIX
COMPAT_CFLAGS += -DRUNTIME_PREFIX
 endif
diff --git a/compat/qsort_s.c b/compat/qsort_s.c
new file mode 100644
index 0..52d1f0a73
--- /dev/null
+++ b/compat/qsort_s.c
@@ -0,0 +1,69 @@
+#include "../git-compat-util.h"
+
+/*
+ * A merge sort implementation, simplified from the qsort implementation
+ * by Mike Haertel, which is a part of the GNU C Library.
+ * Added context pointer, safety checks and return value.
+ */
+
+static void msort_with_tmp(void *b, size_t n, size_t s,
+  int (*cmp)(const void *, const void *, void *),
+  char *t, void *ctx)
+{
+   char *tmp;
+   char *b1, *b2;
+   size_t n1, n2;
+
+   if (n <= 1)
+   return;
+
+   n1 = n / 2;
+   n2 = n - n1;
+   b1 = b;
+   b2 = (char *)b + (n1 * s);
+
+   msort_with_tmp(b1, n1, s, cmp, t, ctx);
+   msort_with_tmp(b2, n2, s, cmp, t, ctx);
+
+   tmp = t;
+
+   while (n1 > 0 && n2 > 0) {
+   if (cmp(b1, b2, ctx) <= 0) {
+   memcpy(tmp, b1, s);
+   tmp += s;
+   b1 += s;
+   --n1;
+   } else {
+   memcpy(tmp, b2, s);
+   tmp += s;
+   b2 += s;
+   --n2;
+   }
+   }
+   if (n1 > 0)
+   memcpy(tmp, b1, n1 * s);
+   memcpy(b, t, (n - n2) * s);
+}
+
+int git_qsort_s(void *b, size_t n, size_t s,
+   int (*cmp)(const void *, const void *, void *), void *ctx)
+{
+   const size_t size = st_mult(n, s);
+   char buf[1024];
+
+   if (!n)
+   return 0;
+   if (!b || !cmp)
+   return -1;
+
+   if (size < sizeof(buf)) {
+   /* The temporary array fits on the small on-stack buffer. */
+   msort_with_tmp(b, n, s, cmp, buf, ctx);
+   } else {
+   /* It's somewhat large, so malloc it.  */
+   char *tmp = xmalloc(size);
+   msort_with_tmp(b, n, s, cmp, tmp, ctx);
+   free(tmp);
+   }
+   return 0;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092..d25f0bd4c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -988,6 +988,12 @@ static inline void sane_qsort(void *base, size_t nmemb, 
size_t size,
qsort(base, nmemb, size, compar);
 }
 
+#ifndef HAVE_QSORT_S
+int git_qsort_s(void *base, size_t nmemb, size_t size,
+   int (*compar)(const void *, const void *, void *), void *ctx);
+#define qsort_s git_qsort_s
+#endif
+
 #ifndef REG_STARTEND
 #error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
 #endif
-- 
2.11.0

[PATCH 0/3] string-list: make string_list_sort() reentrant

[René Scharfe]

Use qsort_s() from C11 Annex K to make string_list_sort() safer, in
particular when called from parallel threads.

  compat: add qsort_s()
  add QSORT_S
  string-list: use QSORT_S in string_list_sort()

 Makefile  | 10 
 compat/qsort_s.c  | 69 +++
 git-compat-util.h | 11 +
 string-list.c | 13 ---
 4 files changed, 95 insertions(+), 8 deletions(-)
 create mode 100644 compat/qsort_s.c

-- 
2.11.0

[PATCH] sha1_name: make wraparound of the index into ring-buffer explicit

[René Scharfe]

Overflow is defined for unsigned integers, but not for signed ones.
Wrap around explicitly for the new ring-buffer in find_unique_abbrev()
as we did in bb84735c for the ones in sha1_to_hex() and get_pathname(),
thus avoiding signed overflows and getting rid of the magic number 3.

Signed-off-by: Rene Scharfe 
---
 sha1_name.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index 06409a3..73a915f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -510,7 +510,8 @@ const char *find_unique_abbrev(const unsigned char *sha1, 
int len)
 {
static int bufno;
static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
-   char *hex = hexbuffer[3 & ++bufno];
+   char *hex = hexbuffer[bufno];
+   bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
find_unique_abbrev_r(hex, sha1, len);
return hex;
 }
-- 
2.10.2

[PATCH RESEND] cocci: avoid self-references in object_id transformations

[René Scharfe]

The object_id functions oid_to_hex, oid_to_hex_r, oidclr, oidcmp, and
oidcpy are defined as wrappers of their legacy counterparts sha1_to_hex,
sha1_to_hex_r, hashclr, hashcmp, and hashcpy, respectively.  Make sure
that the Coccinelle transformations for converting legacy function calls
are not applied to these wrappers themselves, which would result in
tautological declarations.

Signed-off-by: Rene Scharfe 
---
 contrib/coccinelle/object_id.cocci | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/contrib/coccinelle/object_id.cocci 
b/contrib/coccinelle/object_id.cocci
index 0307624..09afdbf 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -17,10 +17,13 @@ expression E1;
 + oid_to_hex()
 
 @@
+identifier f != oid_to_hex;
 expression E1;
 @@
+  f(...) {...
 - sha1_to_hex(E1->hash)
 + oid_to_hex(E1)
+  ...}
 
 @@
 expression E1, E2;
@@ -29,10 +32,13 @@ expression E1, E2;
 + oid_to_hex_r(E1, )
 
 @@
+identifier f != oid_to_hex_r;
 expression E1, E2;
 @@
+   f(...) {...
 - sha1_to_hex_r(E1, E2->hash)
 + oid_to_hex_r(E1, E2)
+  ...}
 
 @@
 expression E1;
@@ -41,10 +47,13 @@ expression E1;
 + oidclr()
 
 @@
+identifier f != oidclr;
 expression E1;
 @@
+  f(...) {...
 - hashclr(E1->hash)
 + oidclr(E1)
+  ...}
 
 @@
 expression E1, E2;
@@ -53,10 +62,13 @@ expression E1, E2;
 + oidcmp(, )
 
 @@
+identifier f != oidcmp;
 expression E1, E2;
 @@
+  f(...) {...
 - hashcmp(E1->hash, E2->hash)
 + oidcmp(E1, E2)
+  ...}
 
 @@
 expression E1, E2;
@@ -77,10 +89,13 @@ expression E1, E2;
 + oidcpy(, )
 
 @@
+identifier f != oidcpy;
 expression E1, E2;
 @@
+  f(...) {...
 - hashcpy(E1->hash, E2->hash)
 + oidcpy(E1, E2)
+  ...}
 
 @@
 expression E1, E2;
-- 
2.10.2

[PATCH] commit: simplify building parents list

[René Scharfe]

Push pptr down into the FROM_MERGE branch of the if/else statement,
where it's actually used, and call commit_list_append() for appending
elements instead of playing tricks with commit_list_insert().  Call
copy_commit_list() in the amend branch instead of open-coding it.  Don't
bother setting pptr in the final branch as it's not used thereafter.

Signed-off-by: Rene Scharfe 
---
 builtin/commit.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a2baa6e..8976c3d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1642,7 +1642,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
-   struct commit_list *parents = NULL, **pptr = 
+   struct commit_list *parents = NULL;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
@@ -1688,20 +1688,18 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
if (!reflog_msg)
reflog_msg = "commit (initial)";
} else if (amend) {
-   struct commit_list *c;
-
if (!reflog_msg)
reflog_msg = "commit (amend)";
-   for (c = current_head->parents; c; c = c->next)
-   pptr = _list_insert(c->item, pptr)->next;
+   parents = copy_commit_list(current_head->parents);
} else if (whence == FROM_MERGE) {
struct strbuf m = STRBUF_INIT;
FILE *fp;
int allow_fast_forward = 1;
+   struct commit_list **pptr = 
 
if (!reflog_msg)
reflog_msg = "commit (merge)";
-   pptr = _list_insert(current_head, pptr)->next;
+   pptr = commit_list_append(current_head, pptr);
fp = fopen(git_path_merge_head(), "r");
if (fp == NULL)
die_errno(_("could not open '%s' for reading"),
@@ -1712,7 +1710,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
parent = get_merge_parent(m.buf);
if (!parent)
die(_("Corrupt MERGE_HEAD file (%s)"), m.buf);
-   pptr = _list_insert(parent, pptr)->next;
+   pptr = commit_list_append(parent, pptr);
}
fclose(fp);
strbuf_release();
@@ -1729,7 +1727,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
reflog_msg = (whence == FROM_CHERRY_PICK)
? "commit (cherry-pick)"
: "commit";
-   pptr = _list_insert(current_head, pptr)->next;
+   commit_list_insert(current_head, );
}
 
/* Finally, get the commit message */
-- 
2.10.2

Re: [PATCH] valgrind: support test helpers

[René Scharfe]

Am 28.10.2016 um 10:51 schrieb Johannes Schindelin:

On Fri, 28 Oct 2016, René Scharfe wrote:

Signed-off-by: Rene Scharfe <l@web.de>


Apart from the missing accent ("é") in your SOB: ACK.


I sign off without accent out of habit, to avoid display problems -- 
better have a plain "e" than something like "" or worse.


But only now I realized that I can fix at least my end by using an UTF-8 
locale (e.g. LANG=C.UTF-8 instead of LANG=C) -- together with PuTTY and 
its settings Window, Translation, Remote character set: UTF-8 and 
Connection, Data, Terminal-type-string: linux, which I already had.  My 
terminal just got boosted into the 21st century, woohoo! ;)


René

Re: [PATCH] valgrind: support test helpers

[René Scharfe]

Am 28.10.2016 um 14:50 schrieb Junio C Hamano:
> Hmph.  I somehow thought this was supposed to have been fixed by
> 503e224180 ("t/test-lib.sh: fix running tests with --valgrind",
> 2016-07-11) already.

Its title seems to indicate that intention.  Probably the quickest test
script that calls a helper is t0009-prio-queue.sh, and without my patch
it reports something like this, unfortunately:

  expecting success:
  test-prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
  test_cmp expect actual
  
  ./t0009-prio-queue.sh: 4: eval: test-prio-queue: not found
  not ok 1 - basic ordering
  #
  #   test-prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
  #   test_cmp expect actual
  #

René

[PATCH] valgrind: support test helpers

[René Scharfe]

Tests run with --valgrind call git commands through a wrapper script
that invokes valgrind on them.  This script (valgrind.sh) is in turn
invoked through symlinks created for each command in t/valgrind/bin/.

Since e6e7530d (test helpers: move test-* to t/helper/ subdirectory)
these symlinks have been broken for test helpers -- they point to the
old locations in the root of the build directory.  Fix that by teaching
the code for creating the links about the new location of the binaries,
and do the same in the wrapper script to allow it to find its payload.

Signed-off-by: Rene Scharfe 
---
 t/test-lib.sh  |  9 -
 t/valgrind/valgrind.sh | 12 ++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index b859db6..a724181 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -809,7 +809,14 @@ then
return;
 
base=$(basename "$1")
-   symlink_target=$GIT_BUILD_DIR/$base
+   case "$base" in
+   test-*)
+   symlink_target="$GIT_BUILD_DIR/t/helper/$base"
+   ;;
+   *)
+   symlink_target="$GIT_BUILD_DIR/$base"
+   ;;
+   esac
# do not override scripts
if test -x "$symlink_target" &&
test ! -d "$symlink_target" &&
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 4215303..669ebaf 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -1,11 +1,19 @@
 #!/bin/sh
 
 base=$(basename "$0")
+case "$base" in
+test-*)
+   program="$GIT_VALGRIND/../../t/helper/$base"
+   ;;
+*)
+   program="$GIT_VALGRIND/../../$base"
+   ;;
+esac
 
 TOOL_OPTIONS='--leak-check=no'
 
 test -z "$GIT_VALGRIND_ENABLED" &&
-exec "$GIT_VALGRIND"/../../"$base" "$@"
+exec "$program" "$@"
 
 case "$GIT_VALGRIND_MODE" in
 memcheck-fast)
@@ -29,4 +37,4 @@ exec valgrind -q --error-exitcode=126 \
--log-fd=4 \
--input-fd=4 \
$GIT_VALGRIND_OPTIONS \
-   "$GIT_VALGRIND"/../../"$base" "$@"
+   "$program" "$@"
-- 
2.10.1

Re: [PATCH] hex: use unsigned index for ring buffer

[René Scharfe]

Am 25.10.2016 um 20:28 schrieb Junio C Hamano:

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


On Mon, Oct 24, 2016 at 04:53:50PM -0700, Junio C Hamano wrote:


So how about this?  It gets rid of magic number 3 and works for array
size that's not a power of two.  And as a nice side effect it can't
trigger a signed overflow anymore.


Looks good to me.  Peff?


Any of the variants discussed in this thread is fine by me.


OK, here is what I'll queue then.
I assumed that René wants to sign it off ;-).


Actually I didn't sign-off on purpose originally.  But OK, let's keep
the version below.  I just feel strangely sad seeing that concise magic
go.  Nevermind.

Signed-off-by: Rene Scharfe <l@web.de>


-- >8 --
From: René Scharfe <l@web.de>
Date: Sun, 23 Oct 2016 19:57:30 +0200
Subject: [PATCH] hex: make wraparound of the index into ring-buffer explicit

Overflow is defined for unsigned integers, but not for signed ones.

We could make the ring-buffer index in sha1_to_hex() and
get_pathname() unsigned to be on the safe side to resolve this, but
let's make it explicit that we are wrapping around at whatever the
number of elements the ring-buffer has.  The compiler is smart enough
to turn modulus into bitmask for these codepaths that use
ring-buffers of a size that is a power of 2.

Signed-off-by: René Scharfe <l@web.de>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 hex.c  | 3 ++-
 path.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hex.c b/hex.c
index ab2610e498..845b01a874 100644
--- a/hex.c
+++ b/hex.c
@@ -78,7 +78,8 @@ char *sha1_to_hex(const unsigned char *sha1)
 {
static int bufno;
static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
-   return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
+   bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
+   return sha1_to_hex_r(hexbuffer[bufno], sha1);
 }

 char *oid_to_hex(const struct object_id *oid)
diff --git a/path.c b/path.c
index fe3c4d96c6..9bfaeda207 100644
--- a/path.c
+++ b/path.c
@@ -24,7 +24,8 @@ static struct strbuf *get_pathname(void)
STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
};
static int index;
-   struct strbuf *sb = _array[3 & ++index];
+   struct strbuf *sb = _array[index];
+   index = (index + 1) % ARRAY_SIZE(pathname_array);
strbuf_reset(sb);
return sb;
 }

Re: [PATCH] hex: use unsigned index for ring buffer

[René Scharfe]

Am 24.10.2016 um 19:27 schrieb Junio C Hamano:
> Junio C Hamano  writes:
> 
>>> I think it would be preferable to just fix it inline in each place.
>>
>> Yeah, probably.
>>
>> My initial reaction to this was
>>
>>  char *sha1_to_hex(const unsigned char *sha1)
>>  {
>> -static int bufno;
>> +static unsigned int bufno;
>>  static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
>>  return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
>>
>> "ah, we do not even need bufno as uint; it could be ushort or even
>> uchar".  If this were a 256 element ring buffer and the index were
>> uchar, we wouldn't even be having this discussion, and "3 &" is a
>> way to get a fake type that is a 2-bit unsigned integer that wraps
>> around when incremented.
>>
>> But being explicit, especially when we know that we can rely on the
>> fact that the compilers are usually intelligent enough, is a good
>> idea, I would think.
>>
>> Isn't size_t often wider than uint, by the way?  It somehow makes me
>> feel dirty to use it when we know we only care about the bottom two
>> bit, especially with the explicit "bufno %= ARRAY_SIZE(hexbuffer)",
>> but I may be simply superstitious in this case.  I dunno.
> 
> If we are doing the wrap-around ourselves, I suspect that the index
> should stay "int" (not even unsigned), as that is supposed to be the
> most natural and performant type on the architecture.  Would it
> still result in better code to use size_t instead?

Right.

Gcc emits an extra cltq instruction for x86-64 (Convert Long To Quad,
i.e. 32-bit to 64-bit) if we wrap explicitly and still use an int in
sha1_to_hex().  It doesn't if we use an unsigned int or size_t.  It
also doesn't for get_pathname().  I guess updating the index variable
only after use makes the difference there.

But I think we can ignore that; it's just an extra cycle.  I only
even noticed it when verifying that "% 4" is turned into "& 3"..
Clang and icc don't add the cltq, by the way.

So how about this?  It gets rid of magic number 3 and works for array
size that's not a power of two.  And as a nice side effect it can't
trigger a signed overflow anymore.

Just adding "unsigned" looks more attractive to me for some reason.
Perhaps I stared enough at the code to get used to the magic values
there..

René

---
 hex.c  | 3 ++-
 path.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hex.c b/hex.c
index ab2610e..845b01a 100644
--- a/hex.c
+++ b/hex.c
@@ -78,7 +78,8 @@ char *sha1_to_hex(const unsigned char *sha1)
 {
static int bufno;
static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
-   return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
+   bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
+   return sha1_to_hex_r(hexbuffer[bufno], sha1);
 }
 
 char *oid_to_hex(const struct object_id *oid)
diff --git a/path.c b/path.c
index a8e7295..52d889c 100644
--- a/path.c
+++ b/path.c
@@ -25,7 +25,8 @@ static struct strbuf *get_pathname(void)
STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
};
static int index;
-   struct strbuf *sb = _array[3 & ++index];
+   struct strbuf *sb = _array[index];
+   index = (index + 1) % ARRAY_SIZE(pathname_array);
strbuf_reset(sb);
return sb;
 }
-- 
2.10.1