* Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates
From: Jeff King @ 2016-10-05 3:54 UTC (permalink / raw)
To: git, René Scharfe
In-Reply-To: <20161005023455.GA6215@pug.qqx.org>
On Tue, Oct 04, 2016 at 10:34:55PM -0400, Aaron Schrab wrote:
> At 16:36 -0400 03 Oct 2016, Jeff King <peff@peff.net> wrote:
> > On a case-insensitive filesystem, we should realize that
> > "a/objects" and "A/objects" are the same path.
>
> The current repository being on a case-insensitive filesystem doesn't
> guarantee that the alternates are as well.
>
> On the other hand, I suspect that people who use a case-insensitive
> filesystem would be less likely to use names which differ only by case.
True. I don't think we actually have enough information to make the
correct comparison (not only that, but I think that fspathcmp() can
sometimes be fooled by a path which is only partially case-insensitive
due to a case-insensitive filesystem mounted on a case-sensitive one).
Still, I think in practice this is likely to do more good than harm, as
I'd guess that being on a single filesystem is the common case.
-Peff
^ permalink raw reply
* RE: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: writeonce @ 2016-10-05 3:00 UTC (permalink / raw)
To: musl; +Cc: Johannes.Schindelin, git, Jeff King
[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]
<
<
< -------- Original Message --------
< Subject: [musl] Re: Regression: git no longer works with musl libc's
< regex impl
< From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
< Date: Tue, October 04, 2016 9:08 am
< To: Rich Felker <dalias@libc.org>
< Cc: Jeff King <peff@peff.net>, git@vger.kernel.org,
< musl@lists.openwall.com
<
< Hi Rich,
<
< On Tue, 4 Oct 2016, Rich Felker wrote:
<
< > On Tue, Oct 04, 2016 at 11:27:22AM -0400, Jeff King wrote:
< > > On Tue, Oct 04, 2016 at 11:08:48AM -0400, Rich Felker wrote:
< > >
< > > > 1. is nonzero mod page size, it just works; the remainder of the
last
< > > > page reads as zero bytes when mmapped.
< > >
< > > Is that a portable assumption?
< >
< > Yes.
<
< No, it is not. You quote POSIX, but the matter of the fact is that we
use
< a subset of POSIX in order to be able to keep things running on
Windows.
<
As far as I can tell (and as the attached program may help demonstrate),
the above assumption has been valid on all versions of Windows since at
least Windows 2000. In this context, one thing to remember is that the
page-size for the mod operation is 4096, whereas the POSIX page-size
(for the purpose of mmap and mremap) is 65536. Note also that in the
case of file-backed mapped sections, using kernel32.dll or msvcrt.dll or
cygwin/newlib or midipix/musl is of little significance, specifically
since all invoke ZwCreateSection and ZwMapViewOfSection under the hood.
HTH,
midipix
< And quite honestly, there are lots of reasons to keep things running
on
< Windows, and even to favor Windows support over musl support. Over
four
< million reasons: the Git for Windows users.
<
< So rather than getting into an ideological discussion about "broken"
< systems, it would be good to keep things practical, realizing that
those
< users make up a very real chunk of all of Git's users.
<
< As to making NO_REGEX conditional on REG_STARTEND: you are talking
about
< apples and oranges here. NO_REGEX is a Makefile flag, while
REG_STARTEND
< is a C preprocessor macro.
<
< Unless you can convince the rest of the Git developers (you would not
< convince me) to simulate autoconf by compiling an executable every
time
< `make` is run, to determine whether REG_STARTEND is defined, this is a
< no-go.
<
< However, you *can* use autoconf directly, and come up with a patch to
our
< configure.ac that detects the absence of REG_STARTEND and sets
NO_REGEX=1.
<
< Alternatively, you can set NO_REGEX=1 in your config.mak.
<
< Or, if you use one of the auto-detected cases in config.mak.uname, you
< could patch it to set NO_REGEX=1.
<
< And lastly, the best alternative would be to teach musl about
< REG_STARTEND, as it is rather useful a feature.
<
< Ciao,
< Johannes
<
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mmap_file.c --]
[-- Type: text/x-c; name="mmap_file.c";, Size: 1487 bytes --]
#define _XOPEN_SOURCE 500
#define NT_FILE_BACKED_PAGE_SIZE 4096
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <errno.h>
#include <sys/mman.h>
static int test_file_backed_mmap(int size)
{
int fd;
int exsize;
void * addr;
char * test;
char * cap;
char name[12];
/* create test file of desired size */
sprintf(name,"%d.tmp",size);
if ((fd = creat(name,0755)) < 0)
return -1;
if (ftruncate(fd,size))
return -1;
close(fd);
/* write 'W' to all bytes of test file */
if ((fd = open(name,O_RDWR,0)) < 0)
return -1;
if ((addr = mmap(0,size,PROT_WRITE,MAP_SHARED,fd,0)) == MAP_FAILED)
return -1;
close(fd);
test = (char *)addr;
cap = (char *)addr + size;
for (; test<cap; test++)
*test = 'W';
munmap(addr,size);
/* map file for read access */
if ((fd = open(name,O_RDONLY,0)) < 0)
return -1;
if ((addr = mmap(0,size,PROT_READ,MAP_PRIVATE,fd,0)) == MAP_FAILED)
return -1;
close(fd);
/* test */
exsize = size + NT_FILE_BACKED_PAGE_SIZE;
exsize &= ~(NT_FILE_BACKED_PAGE_SIZE - 1);
test = (char *)addr + size;
cap = (char *)addr + exsize;
for (; test<cap; test++)
if (*test)
return -1;
/* clean-up */
munmap(addr,size);
unlink(name);
return 0;
}
int main(void)
{
int i;
for (i=1; i<65536; i++)
if (i % NT_FILE_BACKED_PAGE_SIZE)
if (test_file_backed_mmap(i))
return 2;
return 0;
}
^ permalink raw reply
* Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates
From: Aaron Schrab @ 2016-10-05 2:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20161003203626.styj2vwcmgwnpx4v@sigill.intra.peff.net>
At 16:36 -0400 03 Oct 2016, Jeff King <peff@peff.net> wrote:
>On a case-insensitive filesystem, we should realize that
>"a/objects" and "A/objects" are the same path.
The current repository being on a case-insensitive filesystem doesn't
guarantee that the alternates are as well.
On the other hand, I suspect that people who use a case-insensitive
filesystem would be less likely to use names which differ only by case.
^ permalink raw reply
* Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix
From: SZEDER Gábor @ 2016-10-05 1:33 UTC (permalink / raw)
To: Jeff King, Junio C Hamano
Cc: Leho Kraav, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20160907174841.Horde.Ru1LBEeLKomznlWVG-ZnS-Q@webmail.informatik.kit.edu>
Quoting SZEDER Gábor <szeder@ira.uka.de>:
> Quoting SZEDER Gábor <szeder@ira.uka.de>:
>
>> Version sort with prerelease reordering sometimes puts tagnames in the
>> wrong order, when the common part of two compared tagnames ends with
>> the leading character(s) of one or more configured prerelease
>> suffixes.
>>
>> $ git config --get-all versionsort.prereleaseSuffix
>> -beta
>> $ git tag -l --sort=version:refname v2.1.*
>> v2.1.0-beta-2
>> v2.1.0-beta-3
>> v2.1.0
>> v2.1.0-RC1
>> v2.1.0-RC2
>> v2.1.0-beta-1
>> v2.1.1
>> v2.1.2
>>
>> The reason is that when comparing a pair of tagnames, first
>> versioncmp() looks for the first different character in a pair of
>> tagnames, and then the swap_prereleases() helper function checks for
>> prerelease suffixes _starting at_ that character. Thus, when in the
>> above example the sorting algorithm happens to compare the tagnames
>> "v2.1.0-beta-1" and "v2.1.0-RC2", swap_prereleases() will try to match
>> the suffix "-beta" against "beta-1" to no avail, and the two tagnames
>> erroneously end up being ordered lexicographically.
>>
>> To fix this issue change swap_prereleases() to look for configured
>> prerelease suffixes containing that first different character.
>
> Now, while I believe this is the right thing to do to fix this bug,
> there is a corner case, where multiple configured prerelease suffixes
> might match the same tagname:
>
> $ git config --get-all versionsort.prereleaseSuffix
> -bar
> -baz
> -foo-bar
> $ ~/src/git/git tag -l --sort=version:refname
> v1.0-foo-bar
> v1.0-foo-baz
>
> I.e. when comparing these two tags, both "-bar" and "-foo-bar" would
> match "v1.0-foo-bar", and as "-bar" comes first in the config file,
> it wins, and "v1.0-foo-bar" is ordered first. An argument could be
> made to prefer longer matches, in which case "v1.0-foo-bar" would be
> ordered according to "-foo-bar", i.e. as second. However, I don't
> know what that argument could be, to me neither behavior is better
> than the other, but the implementation of the "longest match counts"
> would certainly be more complicated.
>
> The argument I would make is that this is a pathological corner case
> that doesn't worth worrying about.
After having slept on this a couple of times, I think the longest
matching prerelease suffix should determine the sorting order.
A release tag usually consists of an optional prefix, e.g. 'v' or
'snapshot-', followed by the actual version number, followed by an
optional suffix. In the contrived example quoted above this suffix
is '-foo-bar', thus it feels wrong to match '-bar' against it, when
the user explicitly configured '-foo-bar' as prerelease suffix as
well.
Then here is a more realistic case for sorting based on the longest
matching suffix, where
- a longer suffix starts with the shorter one,
- and the longer suffix comes after the shorter one in the
configuration.
With my patches it looks like this:
$ git -c versionsort.prereleasesuffix=-pre \
-c versionsort.prereleasesuffix=-prerelease \
tag -l --sort=version:refname
v1.0.0-prerelease1
v1.0.0-pre1
v1.0.0-pre2
v1.0.0
Yeah, having both '-pre' and '-prerelease' suffixes seems somewhat
silly, but who knows what similar but more reasonable prerelease
suffixes e.g. non-English speaking users might use in their native
language.
Anyway, my intuition says that any '-prereleaseX' tags should come
after all the '-preX' tags. (Ironically, current git just happens
to get this particular case right.)
My patches get this wrong, because they look for prerelease suffixes
_containing_ the first different character. However, when a '-preX'
and a '-prereleaseX' tag are compared, then the whole '-pre' suffix
is part of the common part, thus it doesn't match, only '-prerelease'
matches.
So, to sort this case right the implementation should
- look for a prerelease suffix containing the first different
character or ending right before the first different character,
(This means that when comparing 'v1.0.0-pre1' and 'v1.0.0-pre2'
swap_prereleases() would match '-pre' in both: no big deal, it
should return "undecided" and let the caller do the right thing
by sorting based on '1' and '2')
- and sort a tag based on the longest matching prerelease suffix.
(In my quoted email above I alluded that its implementation must
be more complicated. No, it turns out that it actually isn't.)
(Just for the record: it's still not 100% foolproof, though. Someone
asking for trouble might use letters instead of numbers to indicate
subsequent prereleases, and might have tags 'v1.0-prea', 'v1.0-preb',
..., 'v1.0-prer', and 'v1.0-prerelease'. In this case the sorting
algorithm might happen to decide to compare 'v1.0-prer' and
'v1.0-prerelease', and since the common part is 'v1.0-prer' it will
fail to recognize the '-pre' suffix. I don't see how this case could
be supported in a sane way, or why it's worth to support it.)
And a final sidenote: sorting based on the longest matching suffix
also allows us to (ab)use version sort with prerelease suffixes to
sort postrelease tags as we please, too:
$ ~/src/git/git -c versionsort.prereleasesuffix=-alpha \
-c versionsort.prereleasesuffix=-beta \
-c versionsort.prereleasesuffix= \
-c versionsort.prereleasesuffix=-gamma \
-c versionsort.prereleasesuffix=-delta \
tag -l --sort=version:refname 'v3.0*'
v3.0-alpha1
v3.0-beta1
v3.0
v3.0-gamma1
v3.0-delta1
So, unless I hear a counterargument, I will submit a reroll with
longest matching suffix determining sort order added on top once I
get around to finish up its commit message.
Best,
Gábor
^ permalink raw reply
* Re: [RFC/PATCH] attr: Document a new possible thread safe API
From: Stefan Beller @ 2016-10-04 23:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <xmqqtwcrr9l6.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 4:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
>> index 92fc32a..940617e 100644
>> --- a/Documentation/technical/api-gitattributes.txt
>> +++ b/Documentation/technical/api-gitattributes.txt
>> @@ -59,7 +59,10 @@ Querying Specific Attributes
>> empty `struct git_attr_check` can be prepared by calling
>> `git_attr_check_alloc()` function and then attributes you want to
>> ask about can be added to it with `git_attr_check_append()`
>> - function.
>> + function. git_attr_check_initl is thread safe, i.e. you can call it
>> + from different threads at the same time; internally however only one
>> + call at a time is processed. If the calls from different threads have
>> + the same arguments, the returned `git_attr_check` may be the same.
>
> I do not think this is enough. Look at the example for _initl() and
> notice that it keeps the "singleton static check that is initialized
> by the very first caller if the caller notices it is NULL" pattern.
>
> One way to hide that may be to pass the address of that singleton
> pointer to _initl(), so that it can do the "has it been initialized?
> If not, let's prepare the thing" under lock.
Oh, I see. Yeah that makes sense.
>
>> @@ -89,15 +92,21 @@ static void setup_check(void)
>>
>> ------------
>> const char *path;
>> + struct git_attr_check *result;
>>
>> setup_check();
>> - git_check_attr(path, check);
>> + result = git_check_attr(path, check);
>
> I haven't formed a firm opinion, but I suspect your thinking might
> be clouded by too much staring at the current implementation that
> has <attr>,<value> pairs inside git_attr_check. Traditionally, the
> attr subsystem used the same type for the query question and the
> query answer the same type, but it does not have to stay to be the
> case at all. Have you considered that we are allowed to make these
> two types distinct?
I thought about that, but as I concluded that the get_all_attrs doesn't need
conversion to a threading environment, we can keep it as is.
When keeping the get_all_attrs as is, we need to keep the data structures
as is, (i.e. key,value pair inside git_check_attr), so introducing a new
data type seemed not useful for the threaded part.
> A caller can share the same question instance
> (i.e. the set of interned <attr>, in git_attr_check) with other
> threads as that is a read-only thing, but each of the callers would
> want to have the result array on its own stack if possible
> (e.g. when asking for a known set of attributes, which is the
> majority of the case) to avoid allocation cost. I'd expect the most
> typical caller to be
>
> static struct git_attr_check *check;
> struct git_attr_result result[2]; /* we want two */
>
> git_attr_check_initl(&check, "crlf", "ident", NULL);
> git_check_attr(path, check, result);
> /* result[0] has "crlf", result[1] has "ident" */
>
> or something like that.
I see, that seems to be a clean API. So git_attr_check_initl
will lock the mutex and once it got the mutex it can either
* return early as someone else did the work
* needs to do the actual work
and then unlock. In any case the work was done.
git_check_attr, which runs in all threads points to the same check,
but gets the different results.
Ok, I'll go in that direction then.
Thanks,
Stefan
^ permalink raw reply
* Re: [RFC/PATCH] attr: Document a new possible thread safe API
From: Junio C Hamano @ 2016-10-04 23:13 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill
In-Reply-To: <20161004221433.23747-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
> index 92fc32a..940617e 100644
> --- a/Documentation/technical/api-gitattributes.txt
> +++ b/Documentation/technical/api-gitattributes.txt
> @@ -59,7 +59,10 @@ Querying Specific Attributes
> empty `struct git_attr_check` can be prepared by calling
> `git_attr_check_alloc()` function and then attributes you want to
> ask about can be added to it with `git_attr_check_append()`
> - function.
> + function. git_attr_check_initl is thread safe, i.e. you can call it
> + from different threads at the same time; internally however only one
> + call at a time is processed. If the calls from different threads have
> + the same arguments, the returned `git_attr_check` may be the same.
I do not think this is enough. Look at the example for _initl() and
notice that it keeps the "singleton static check that is initialized
by the very first caller if the caller notices it is NULL" pattern.
One way to hide that may be to pass the address of that singleton
pointer to _initl(), so that it can do the "has it been initialized?
If not, let's prepare the thing" under lock.
> @@ -89,15 +92,21 @@ static void setup_check(void)
>
> ------------
> const char *path;
> + struct git_attr_check *result;
>
> setup_check();
> - git_check_attr(path, check);
> + result = git_check_attr(path, check);
I haven't formed a firm opinion, but I suspect your thinking might
be clouded by too much staring at the current implementation that
has <attr>,<value> pairs inside git_attr_check. Traditionally, the
attr subsystem used the same type for the query question and the
query answer the same type, but it does not have to stay to be the
case at all. Have you considered that we are allowed to make these
two types distinct? A caller can share the same question instance
(i.e. the set of interned <attr>, in git_attr_check) with other
threads as that is a read-only thing, but each of the callers would
want to have the result array on its own stack if possible
(e.g. when asking for a known set of attributes, which is the
majority of the case) to avoid allocation cost. I'd expect the most
typical caller to be
static struct git_attr_check *check;
struct git_attr_result result[2]; /* we want two */
git_attr_check_initl(&check, "crlf", "ident", NULL);
git_check_attr(path, check, result);
/* result[0] has "crlf", result[1] has "ident" */
or something like that.
^ permalink raw reply
* Re: GL bug: can not commit, reports error on changed submodule directory
From: Stefan Beller @ 2016-10-04 23:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: ern0, git@vger.kernel.org
In-Reply-To: <xmqqy423rabi.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ern0 <ern0@linkbroker.hu> writes:
>
>> When I say:
>> $ gl commit -m "blah blah"
>> It reports:
>> ✘ Failed to read file into stream: Is a directory
>
> Not that I am interested in learning the answer to the question, but
> what the h*ck is "gl"?
http://gitless.com/
See 20160930191413.002049b94b3908b15881b77f@domain007.com
"Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
^ permalink raw reply
* Re: GL bug: can not commit, reports error on changed submodule directory
From: Junio C Hamano @ 2016-10-04 22:58 UTC (permalink / raw)
To: ern0; +Cc: git
In-Reply-To: <CALhephTkohVhEjdP7TwQAcBrEBiHGtp0Hd+UxPUiJHtubMWKGA@mail.gmail.com>
ern0 <ern0@linkbroker.hu> writes:
> When I say:
> $ gl commit -m "blah blah"
> It reports:
> ✘ Failed to read file into stream: Is a directory
Not that I am interested in learning the answer to the question, but
what the h*ck is "gl"?
^ permalink raw reply
* [PATCH 3/4] diff.c: move ws-error-highlight parsing helpers up
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <20161004225449.6759-1-gitster@pobox.com>
These need to be usable from git_diff_ui_config() code to help
parsing a configuration variable, so move them up.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff.c | 74 +++++++++++++++++++++++++++++++++---------------------------------
1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/diff.c b/diff.c
index d346378600..bd625cf3f7 100644
--- a/diff.c
+++ b/diff.c
@@ -163,6 +163,43 @@ long parse_algorithm_value(const char *value)
return -1;
}
+static int parse_one_token(const char **arg, const char *token)
+{
+ const char *rest;
+ if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
+ *arg = rest;
+ return 1;
+ }
+ return 0;
+}
+
+static int parse_ws_error_highlight(const char *arg)
+{
+ const char *orig_arg = arg;
+ unsigned val = 0;
+
+ while (*arg) {
+ if (parse_one_token(&arg, "none"))
+ val = 0;
+ else if (parse_one_token(&arg, "default"))
+ val = WSEH_NEW;
+ else if (parse_one_token(&arg, "all"))
+ val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
+ else if (parse_one_token(&arg, "new"))
+ val |= WSEH_NEW;
+ else if (parse_one_token(&arg, "old"))
+ val |= WSEH_OLD;
+ else if (parse_one_token(&arg, "context"))
+ val |= WSEH_CONTEXT;
+ else {
+ return -1 - (int)(arg - orig_arg);
+ }
+ if (*arg)
+ arg++;
+ }
+ return val;
+}
+
/*
* These are to give UI layer defaults.
* The core-level commands such as git-diff-files should
@@ -3656,43 +3693,6 @@ static void enable_patch_output(int *fmt) {
*fmt |= DIFF_FORMAT_PATCH;
}
-static int parse_one_token(const char **arg, const char *token)
-{
- const char *rest;
- if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
- *arg = rest;
- return 1;
- }
- return 0;
-}
-
-static int parse_ws_error_highlight(const char *arg)
-{
- const char *orig_arg = arg;
- unsigned val = 0;
-
- while (*arg) {
- if (parse_one_token(&arg, "none"))
- val = 0;
- else if (parse_one_token(&arg, "default"))
- val = WSEH_NEW;
- else if (parse_one_token(&arg, "all"))
- val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
- else if (parse_one_token(&arg, "new"))
- val |= WSEH_NEW;
- else if (parse_one_token(&arg, "old"))
- val |= WSEH_OLD;
- else if (parse_one_token(&arg, "context"))
- val |= WSEH_CONTEXT;
- else {
- return -1 - (int)(arg - orig_arg);
- }
- if (*arg)
- arg++;
- }
- return val;
-}
-
static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
{
int val = parse_ws_error_highlight(arg);
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply related
* [PATCH 4/4] diff: introduce diff.wsErrorHighlight option
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <20161004225449.6759-1-gitster@pobox.com>
With the preparatory steps, it has become trivial to teach the
system a new diff.wsErrorHighlight configuration that gives the
default value for --ws-error-highlight command line option.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/diff-config.txt | 6 ++++++
Documentation/diff-options.txt | 2 ++
diff.c | 11 ++++++++++-
t/t4015-diff-whitespace.sh | 35 +++++++++++++++++++++++++++++++++++
4 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 6eaa45271c..c0396e66a5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -182,3 +182,9 @@ diff.algorithm::
low-occurrence common elements".
--
+
+
+diff.wsErrorHighlight::
+ A comma separated list of `old`, `new`, `context`, that
+ specifies how whitespace errors on lines are highlighted
+ with `color.diff.whitespace`. Can be overridden by the
+ command line option `--ws-error-highlight=<kind>`
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 306b7e3604..dfd6bc99c6 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -303,6 +303,8 @@ ifndef::git-format-patch[]
lines are highlighted. E.g. `--ws-error-highlight=new,old`
highlights whitespace errors on both deleted and added lines.
`all` can be used as a short-hand for `old,new,context`.
+ The `diff.wsErrorHighlight` configuration variable can be
+ used to specify the default behaviour.
endif::git-format-patch[]
diff --git a/diff.c b/diff.c
index bd625cf3f7..9acf04f5b0 100644
--- a/diff.c
+++ b/diff.c
@@ -41,6 +41,7 @@ static int diff_stat_graph_width;
static int diff_dirstat_permille_default = 30;
static struct diff_options default_diff_options;
static long diff_algorithm;
+static unsigned ws_error_highlight_default = WSEH_NEW;
static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_RESET,
@@ -262,6 +263,14 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp(var, "diff.wserrorhighlight")) {
+ int val = parse_ws_error_highlight(value);
+ if (val < 0)
+ return -1;
+ ws_error_highlight_default = val;
+ return 0;
+ }
+
if (git_color_config(var, value, cb) < 0)
return -1;
@@ -3306,7 +3315,7 @@ void diff_setup(struct diff_options *options)
options->rename_limit = -1;
options->dirstat_permille = diff_dirstat_permille_default;
options->context = diff_context_default;
- options->ws_error_highlight = WSEH_NEW;
+ options->ws_error_highlight = ws_error_highlight_default;
DIFF_OPT_SET(options, RENAME_EMPTY);
/* pathchange left =NULL by default */
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 4a4f374824..289806d0c7 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -937,4 +937,39 @@ test_expect_success 'test --ws-error-highlight option' '
'
+test_expect_success 'test diff.wsErrorHighlight config' '
+
+ git -c color.diff=always -c diff.wsErrorHighlight=default,old diff |
+ test_decode_color >current &&
+ test_cmp expect.default-old current &&
+
+ git -c color.diff=always -c diff.wsErrorHighlight=all diff |
+ test_decode_color >current &&
+ test_cmp expect.all current &&
+
+ git -c color.diff=always -c diff.wsErrorHighlight=none diff |
+ test_decode_color >current &&
+ test_cmp expect.none current
+
+'
+
+test_expect_success 'option overrides diff.wsErrorHighlight' '
+
+ git -c color.diff=always -c diff.wsErrorHighlight=none \
+ diff --ws-error-highlight=default,old |
+ test_decode_color >current &&
+ test_cmp expect.default-old current &&
+
+ git -c color.diff=always -c diff.wsErrorHighlight=default \
+ diff --ws-error-highlight=all |
+ test_decode_color >current &&
+ test_cmp expect.all current &&
+
+ git -c color.diff=always -c diff.wsErrorHighlight=all \
+ diff --ws-error-highlight=none |
+ test_decode_color >current &&
+ test_cmp expect.none current
+
+'
+
test_done
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply related
* [PATCH 2/4] diff.c: refactor parse_ws_error_highlight()
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <20161004225449.6759-1-gitster@pobox.com>
Rename the function to parse_ws_error_highlight_opt(), because it is
meant to parse a command line option, and then refactor the meat of
the function into a helper function that reports the parsed result
which is typically a small unsigned int (these are OR'ed bitmask
after all), or a negative offset that indicates where in the input
string a parse error happened.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/diff.c b/diff.c
index 46260ed7a1..d346378600 100644
--- a/diff.c
+++ b/diff.c
@@ -3666,10 +3666,11 @@ static int parse_one_token(const char **arg, const char *token)
return 0;
}
-static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)
+static int parse_ws_error_highlight(const char *arg)
{
const char *orig_arg = arg;
unsigned val = 0;
+
while (*arg) {
if (parse_one_token(&arg, "none"))
val = 0;
@@ -3684,13 +3685,23 @@ static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)
else if (parse_one_token(&arg, "context"))
val |= WSEH_CONTEXT;
else {
- error("unknown value after ws-error-highlight=%.*s",
- (int)(arg - orig_arg), orig_arg);
- return 0;
+ return -1 - (int)(arg - orig_arg);
}
if (*arg)
arg++;
}
+ return val;
+}
+
+static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
+{
+ int val = parse_ws_error_highlight(arg);
+
+ if (val < 0) {
+ error("unknown value after ws-error-highlight=%.*s",
+ -1 - val, arg);
+ return 0;
+ }
opt->ws_error_highlight = val;
return 1;
}
@@ -3894,7 +3905,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
else if (skip_prefix(arg, "--submodule=", &arg))
return parse_submodule_opt(options, arg);
else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
- return parse_ws_error_highlight(options, arg);
+ return parse_ws_error_highlight_opt(options, arg);
/* misc options */
else if (!strcmp(arg, "-z"))
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply related
* [PATCH 1/4] t4015: split out the "setup" part of ws-error-highlight test
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <20161004225449.6759-1-gitster@pobox.com>
We'd want to run this same set of test twice, once with the option
and another time with an equivalent configuration setting. Split
out the step that prepares the test data and expected output and
move the test for the command line option into a separate test.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t4015-diff-whitespace.sh | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 2434157aa7..4a4f374824 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -869,7 +869,8 @@ test_expect_success 'diff that introduces and removes ws breakages' '
test_cmp expected current
'
-test_expect_success 'the same with --ws-error-highlight' '
+test_expect_success 'ws-error-highlight test setup' '
+
git reset --hard &&
{
echo "0. blank-at-eol " &&
@@ -882,10 +883,7 @@ test_expect_success 'the same with --ws-error-highlight' '
echo "2. and a new line "
} >x &&
- git -c color.diff=always diff --ws-error-highlight=default,old |
- test_decode_color >current &&
-
- cat >expected <<-\EOF &&
+ cat >expect.default-old <<-\EOF &&
<BOLD>diff --git a/x b/x<RESET>
<BOLD>index d0233a2..700886e 100644<RESET>
<BOLD>--- a/x<RESET>
@@ -897,12 +895,7 @@ test_expect_success 'the same with --ws-error-highlight' '
<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
EOF
- test_cmp expected current &&
-
- git -c color.diff=always diff --ws-error-highlight=all |
- test_decode_color >current &&
-
- cat >expected <<-\EOF &&
+ cat >expect.all <<-\EOF &&
<BOLD>diff --git a/x b/x<RESET>
<BOLD>index d0233a2..700886e 100644<RESET>
<BOLD>--- a/x<RESET>
@@ -914,12 +907,7 @@ test_expect_success 'the same with --ws-error-highlight' '
<GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET>
EOF
- test_cmp expected current &&
-
- git -c color.diff=always diff --ws-error-highlight=none |
- test_decode_color >current &&
-
- cat >expected <<-\EOF &&
+ cat >expect.none <<-\EOF
<BOLD>diff --git a/x b/x<RESET>
<BOLD>index d0233a2..700886e 100644<RESET>
<BOLD>--- a/x<RESET>
@@ -931,7 +919,22 @@ test_expect_success 'the same with --ws-error-highlight' '
<GREEN>+2. and a new line <RESET>
EOF
- test_cmp expected current
+'
+
+test_expect_success 'test --ws-error-highlight option' '
+
+ git -c color.diff=always diff --ws-error-highlight=default,old |
+ test_decode_color >current &&
+ test_cmp expect.default-old current &&
+
+ git -c color.diff=always diff --ws-error-highlight=all |
+ test_decode_color >current &&
+ test_cmp expect.all current &&
+
+ git -c color.diff=always diff --ws-error-highlight=none |
+ test_decode_color >current &&
+ test_cmp expect.none current
+
'
test_done
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply related
* [PATCH 0/4] diff.wsErrorHighlight configuration variable
From: Junio C Hamano @ 2016-10-04 22:54 UTC (permalink / raw)
To: git; +Cc: peff, strk
In-Reply-To: <xmqqk2douhe0.fsf@gitster.mtv.corp.google.com>
"git diff" and its family of commands have "--ws-error-highlight"
option to allow whitespace breakages on old and context lines
painted in color.diff.whitespace color, instead of the usual "we
paint breakages only on new lines", but there wasn't a configuration
variable that corresponds to it.
This would be a lot closer to a series that could be acceptable,
compared to the previous "it should look like this" patch.
Junio C Hamano (4):
t4015: split out the "setup" part of ws-error-highlight test
diff.c: refactor parse_ws_error_highlight()
diff.c: move ws-error-highlight parsing helpers up
diff: introduce diff.wsErrorHighlight option
Documentation/diff-config.txt | 6 +++
Documentation/diff-options.txt | 2 +
diff.c | 88 ++++++++++++++++++++++++++----------------
t/t4015-diff-whitespace.sh | 74 ++++++++++++++++++++++++++---------
4 files changed, 118 insertions(+), 52 deletions(-)
--
2.10.1-510-g1ef781f2c1
^ permalink raw reply
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: Junio C Hamano @ 2016-10-04 22:48 UTC (permalink / raw)
To: Rich Felker; +Cc: James B, musl, Johannes Schindelin, Jeff King, git
In-Reply-To: <20161004223322.GE19318@brightrain.aerifal.cx>
Rich Felker <dalias@libc.org> writes:
> This is especially unfriendly when the semantics of the switch come
> across, at least to some users, as "your system regex is incomplete"
> rather than "git can't use it because git depends on nonstandard
> extensions".
The latter is exactly what Makefile patch that brought this change
says, I think.
# Define NO_REGEX if your C library lacks regex support with REG_STARTEND
# feature.
Before the series updated the message to the above, we used to say:
# Define NO_REGEX if you have no or inferior regex support in your C library.
which _was_ unfair to those who needed to set NO_REGEX for whatever
reason. It was totally unclear "inferior" relative to what standard
the message was passing its harsh judgement on your C library.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #08; Tue, 27)
From: Stefan Beller @ 2016-10-04 22:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqh98sw0tv.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 9:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> The question git_all_attrs() asks is quite different.
And as far as I can tell it is only used by builtin/check-attr.c which we do not
intend to convert into a threaded beast in the foreseeable future I'd expect.
So maybe we can just punt on redefining the API there, i.e.
we keep the question and answer together in a struct git_attr_check.
<some return value here>
git_all_attr(const char *path); /* no other arguments except path*/
That looks great, and I incorporated that into the proposal I sent out
as an RFC.
^ permalink raw reply
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: Rich Felker @ 2016-10-04 22:33 UTC (permalink / raw)
To: James B; +Cc: musl, Johannes Schindelin, Jeff King, git
In-Reply-To: <20161005090625.683fdbbfac8164125dee6469@gmail.com>
On Wed, Oct 05, 2016 at 09:06:25AM +1100, James B wrote:
> On Tue, 4 Oct 2016 18:08:33 +0200 (CEST)
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> >
> > No, it is not. You quote POSIX, but the matter of the fact is that we use
> > a subset of POSIX in order to be able to keep things running on Windows.
> >
> > And quite honestly, there are lots of reasons to keep things running on
> > Windows, and even to favor Windows support over musl support. Over four
> > million reasons: the Git for Windows users.
> >
>
> Wow, I don't know that Windows is a git's first-tier platform now,
> and Linux/POSIX second. Are we talking about the same git that was
> originally written in Linus Torvalds, and is used to manage Linux
> kernel? Are you by any chance employed by Redmond, directly or
> indirectly?
>
> Sorry - can't help it.
I don't think the hostility and sarcasm are really needed here. But
what this does speak to is that users don't like feeling like their
platform is being treated as a second-class target, which is what it
feels like when you have to manually flip a switch to make git build.
This is especially unfriendly when the semantics of the switch come
across, at least to some users, as "your system regex is incomplete"
rather than "git can't use it because git depends on nonstandard
extensions".
Rich
^ permalink raw reply
* [RFC/PATCH] attr: Document a new possible thread safe API
From: Stefan Beller @ 2016-10-04 22:14 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, Stefan Beller
This is what we want to see at the end of the refactoring session
to enable the attr subsystem to be thread safe.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Junio wrote:
>> So how would we go about git_all_attrs then?
>
> I think you arrived the same conclusion, but ...
I think the changes as proposed are minimal to be able to use attrs in a
multithreaded environment.
This patch applies on top of jc/attr-more.
A complete diff between origin/master..HEAD for the documentation is
appended below.
Thanks,
Stefan
Documentation/technical/api-gitattributes.txt | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 92fc32a..940617e 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -59,7 +59,10 @@ Querying Specific Attributes
empty `struct git_attr_check` can be prepared by calling
`git_attr_check_alloc()` function and then attributes you want to
ask about can be added to it with `git_attr_check_append()`
- function.
+ function. git_attr_check_initl is thread safe, i.e. you can call it
+ from different threads at the same time; internally however only one
+ call at a time is processed. If the calls from different threads have
+ the same arguments, the returned `git_attr_check` may be the same.
* Call `git_check_attr()` to check the attributes for the path.
@@ -89,15 +92,21 @@ static void setup_check(void)
------------
const char *path;
+ struct git_attr_check *result;
setup_check();
- git_check_attr(path, check);
+ result = git_check_attr(path, check);
------------
-. Act on `.value` member of the result, left in `check->check[]`:
+The result may be the same as `check` (in a single threaded application),
+but generally assume it is not. The `result` must not be free'd as it is
+owned by the attr subsystem. It is reused by the same thread, so a subsequent
+call to git_check_attr in the same thread will overwrite the result.
+
+. Act on `.value` member of the `result->check[]`:
------------
- const char *value = check->check[0].value;
+ const char *value = result->check[0].value;
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -138,10 +147,7 @@ Querying All Attributes
To get the values of all attributes associated with a file:
-* Prepare an empty `git_attr_check` structure by calling
- `git_attr_check_alloc()`.
-
-* Call `git_all_attrs()`, which populates the `git_attr_check`
+* Call `git_all_attrs()`, which returns a `git_attr_check`
with the attributes attached to the path.
* Iterate over the `git_attr_check.check[]` array to examine
--
2.10.0.129.g35f6318
The following is `git diff origin/master Documentation/technical/api-gitattributes.txt`:
diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 2602668..940617e 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -16,10 +16,15 @@ Data Structure
of no interest to the calling programs. The name of the
attribute can be retrieved by calling `git_attr_name()`.
+`struct git_attr_check_elem`::
+
+ This structure represents one attribute and its value.
+
`struct git_attr_check`::
- This structure represents a set of attributes to check in a call
- to `git_check_attr()` function, and receives the results.
+ This structure represents a collection of `git_attr_check_elem`.
+ It is passed to `git_check_attr()` function, specifying the
+ attributes to check, and receives their values.
Attribute Values
@@ -48,49 +53,60 @@ value of the attribute for the path.
Querying Specific Attributes
----------------------------
-* Prepare an array of `struct git_attr_check` to define the list of
- attributes you would want to check. To populate this array, you would
- need to define necessary attributes by calling `git_attr()` function.
+* Prepare `struct git_attr_check` using git_attr_check_initl()
+ function, enumerating the names of attributes whose values you are
+ interested in, terminated with a NULL pointer. Alternatively, an
+ empty `struct git_attr_check` can be prepared by calling
+ `git_attr_check_alloc()` function and then attributes you want to
+ ask about can be added to it with `git_attr_check_append()`
+ function. git_attr_check_initl is thread safe, i.e. you can call it
+ from different threads at the same time; internally however only one
+ call at a time is processed. If the calls from different threads have
+ the same arguments, the returned `git_attr_check` may be the same.
* Call `git_check_attr()` to check the attributes for the path.
-* Inspect `git_attr_check` structure to see how each of the attribute in
- the array is defined for the path.
+* Inspect `git_attr_check` structure to see how each of the
+ attribute in the array is defined for the path.
Example
-------
-To see how attributes "crlf" and "indent" are set for different paths.
+To see how attributes "crlf" and "ident" are set for different paths.
-. Prepare an array of `struct git_attr_check` with two elements (because
- we are checking two attributes). Initialize their `attr` member with
- pointers to `struct git_attr` obtained by calling `git_attr()`:
+. Prepare a `struct git_attr_check` with two elements (because
+ we are checking two attributes):
------------
-static struct git_attr_check check[2];
+static struct git_attr_check *check;
static void setup_check(void)
{
- if (check[0].attr)
+ if (check)
return; /* already done */
- check[0].attr = git_attr("crlf");
- check[1].attr = git_attr("ident");
+ check = git_attr_check_initl("crlf", "ident", NULL);
}
------------
-. Call `git_check_attr()` with the prepared array of `struct git_attr_check`:
+. Call `git_check_attr()` with the prepared `struct git_attr_check`:
------------
const char *path;
+ struct git_attr_check *result;
setup_check();
- git_check_attr(path, ARRAY_SIZE(check), check);
+ result = git_check_attr(path, check);
------------
-. Act on `.value` member of the result, left in `check[]`:
+The result may be the same as `check` (in a single threaded application),
+but generally assume it is not. The `result` must not be free'd as it is
+owned by the attr subsystem. It is reused by the same thread, so a subsequent
+call to git_check_attr in the same thread will overwrite the result.
+
+. Act on `.value` member of the `result->check[]`:
------------
- const char *value = check[0].value;
+ const char *value = result->check[0].value;
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -109,20 +125,36 @@ static void setup_check(void)
}
------------
+To see how attributes in argv[] are set for different paths, only
+the first step in the above would be different.
+
+------------
+static struct git_attr_check *check;
+static void setup_check(const char **argv)
+{
+ check = git_attr_check_alloc();
+ while (*argv) {
+ struct git_attr *attr = git_attr(*argv);
+ git_attr_check_append(check, attr);
+ argv++;
+ }
+}
+------------
+
Querying All Attributes
-----------------------
To get the values of all attributes associated with a file:
-* Call `git_all_attrs()`, which returns an array of `git_attr_check`
- structures.
+* Call `git_all_attrs()`, which returns a `git_attr_check`
+ with the attributes attached to the path.
-* Iterate over the `git_attr_check` array to examine the attribute
- names and values. The name of the attribute described by a
- `git_attr_check` object can be retrieved via
- `git_attr_name(check[i].attr)`. (Please note that no items will be
- returned for unset attributes, so `ATTR_UNSET()` will return false
- for all returned `git_array_check` objects.)
+* Iterate over the `git_attr_check.check[]` array to examine
+ the attribute names and values. The name of the attribute
+ described by a `git_attr_check.check[]` object can be retrieved via
+ `git_attr_name(check->check[i].attr)`. (Please note that no items
+ will be returned for unset attributes, so `ATTR_UNSET()` will return
+ false for all returned `git_array_check` objects.)
-* Free the `git_array_check` array.
+* Free the `git_array_check` by calling `git_attr_check_free()`.
^ permalink raw reply related
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: James B @ 2016-10-04 22:06 UTC (permalink / raw)
To: musl; +Cc: Johannes Schindelin, Rich Felker, Jeff King, git
In-Reply-To: <alpine.DEB.2.20.1610041802310.35196@virtualbox>
On Tue, 4 Oct 2016 18:08:33 +0200 (CEST)
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> No, it is not. You quote POSIX, but the matter of the fact is that we use
> a subset of POSIX in order to be able to keep things running on Windows.
>
> And quite honestly, there are lots of reasons to keep things running on
> Windows, and even to favor Windows support over musl support. Over four
> million reasons: the Git for Windows users.
>
Wow, I don't know that Windows is a git's first-tier platform now, and Linux/POSIX second. Are we talking about the same git that was originally written in Linus Torvalds, and is used to manage Linux kernel? Are you by any chance employed by Redmond, directly or indirectly?
Sorry - can't help it.
^ permalink raw reply
* Re: [PATCH 12/18] alternates: use a separate scratch space
From: Jeff King @ 2016-10-04 21:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, René Scharfe
In-Reply-To: <xmqq7f9nss1y.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 04, 2016 at 02:49:45PM -0700, Junio C Hamano wrote:
> >> It is not wrong per-se, but I am a bit surprised to see that the
> >> code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed
> >> at by the scratch pointer.
> >
> > Yeah, there's really no reason "path" could not become a non-flex
> > buffer. I mostly left it there out of inertia. If you have a preference,
> > I'm happy to change it.
>
> My preference, before reaching the end of the series, actually was
> to overallocate just once and point with *scratch into path[] beyond
> the end of the fixed "where is the object directory?" string.
>
> Of course, that would not mesh very well with the plan this series
> had after this step to use strbuf for keeping scratch ;-) And the
> end result looks fine to me.
Heh, yeah, I did not think of that (because I had the strbuf end-game in
mind the whole time). I agree that would be nicer if we were keeping the
raw buffer, if only because one could free the whole thing in one shot
(OTOH, we do not ever free these structs at all :) ).
-Peff
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jacob Keller @ 2016-10-04 21:50 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161004214914.kgkot337awszhojs@sigill.intra.peff.net>
On Tue, Oct 4, 2016 at 2:49 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 04, 2016 at 02:43:24PM -0700, Jacob Keller wrote:
>
>> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make
>> > this clear. I put the numbers under the ">" of the arrow. Did I screw up
>> > the spacing somehow so that isn't how they look to you? Or are you just
>> > saying you would prefer them under the "-" of the arrow?
>>
>> I bet they line up in a monospace font and I just happened to be
>> viewing this from GMail which isn't showing it in monospace and so it
>> doesn't line up. Ignore me and carry on
>
> Oh, good. I was wondering if I was going crazy. :)
>
> -Peff
Only one of us is going crazy, but I'm not sure who ;)
-Jake
^ permalink raw reply
* Re: [PATCH 12/18] alternates: use a separate scratch space
From: Junio C Hamano @ 2016-10-04 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20161004213241.ihzkl7cohliavydg@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Oct 04, 2016 at 02:29:46PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > extern struct alternate_object_database {
>> > struct alternate_object_database *next;
>> > +
>> > char *name;
>> > - char base[FLEX_ARRAY]; /* more */
>> > + char *scratch;
>> > +
>> > + char path[FLEX_ARRAY];
>> > } *alt_odb_list;
>>
>> It is not wrong per-se, but I am a bit surprised to see that the
>> code keeps FLEX_ARRAY _and_ uses a separate malloc'ed area pointed
>> at by the scratch pointer.
>
> Yeah, there's really no reason "path" could not become a non-flex
> buffer. I mostly left it there out of inertia. If you have a preference,
> I'm happy to change it.
My preference, before reaching the end of the series, actually was
to overallocate just once and point with *scratch into path[] beyond
the end of the fixed "where is the object directory?" string.
Of course, that would not mesh very well with the plan this series
had after this step to use strbuf for keeping scratch ;-) And the
end result looks fine to me.
Thanks.
^ permalink raw reply
* Re: [PATCH 13/18] fill_sha1_file: write "boring" characters
From: Jacob Keller @ 2016-10-04 21:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Git mailing list, René Scharfe
In-Reply-To: <xmqqbmyzss6z.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 2:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
>>> This function forms a sha1 as "xx/yyyy...", but skips over
>>> the slot for the slash rather than writing it, leaving it to
>>> the caller to do so. It also does not bother to put in a
>>> trailing NUL, even though every caller would want it (we're
>>> forming a path which by definition is not a directory, so
>>> the only thing to do with it is feed it to a system call).
>>>
>>> Let's make the lives of our callers easier by just writing
>>> out the internal "/" and the NUL.
>>> ...
>>
>> I think this makes a lot more sense than making the callers have to do this.
>
> The cost of fill function having to do the same thing repeatedly is
> negligible, so I am OK with the result, but for fairness, this was
> not "make the callers do this extra thing", but was "the caller can
> prepare these unchanging parts just once, and the fill function that
> is repeatedly run does not have to."
>
Sure, but it's a pretty minor optimization and I think the result is
easier to understand.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jeff King @ 2016-10-04 21:49 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xoDz2sOPrDrJhAhrqDQsRR8NVU-8kh6+G=8FJeXEJ1dtg@mail.gmail.com>
On Tue, Oct 04, 2016 at 02:43:24PM -0700, Jacob Keller wrote:
> > Hmm. Now I am puzzled, because I _did_ line up them specifically to make
> > this clear. I put the numbers under the ">" of the arrow. Did I screw up
> > the spacing somehow so that isn't how they look to you? Or are you just
> > saying you would prefer them under the "-" of the arrow?
>
> I bet they line up in a monospace font and I just happened to be
> viewing this from GMail which isn't showing it in monospace and so it
> doesn't line up. Ignore me and carry on
Oh, good. I was wondering if I was going crazy. :)
-Peff
^ permalink raw reply
* Re: [PATCH 13/18] fill_sha1_file: write "boring" characters
From: Jeff King @ 2016-10-04 21:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list, René Scharfe
In-Reply-To: <xmqqbmyzss6z.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 04, 2016 at 02:46:44PM -0700, Junio C Hamano wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
> > On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
> >> This function forms a sha1 as "xx/yyyy...", but skips over
> >> the slot for the slash rather than writing it, leaving it to
> >> the caller to do so. It also does not bother to put in a
> >> trailing NUL, even though every caller would want it (we're
> >> forming a path which by definition is not a directory, so
> >> the only thing to do with it is feed it to a system call).
> >>
> >> Let's make the lives of our callers easier by just writing
> >> out the internal "/" and the NUL.
> >> ...
> >
> > I think this makes a lot more sense than making the callers have to do this.
>
> The cost of fill function having to do the same thing repeatedly is
> negligible, so I am OK with the result, but for fairness, this was
> not "make the callers do this extra thing", but was "the caller can
> prepare these unchanging parts just once, and the fill function that
> is repeatedly run does not have to."
Yeah, perhaps "does not bother" in the commit message is not entirely
fair. But it really does feel like quite a premature optimization to
skip the writing of one "/" in the middle of the string, especially as
it impacts the interface.
-Peff
^ permalink raw reply
* Re: [PATCH 13/18] fill_sha1_file: write "boring" characters
From: Junio C Hamano @ 2016-10-04 21:46 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jeff King, Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xpOxoRBDZGF_CU1Q-SYiQZtMx2vuwQKS0og864awZod5g@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
>> This function forms a sha1 as "xx/yyyy...", but skips over
>> the slot for the slash rather than writing it, leaving it to
>> the caller to do so. It also does not bother to put in a
>> trailing NUL, even though every caller would want it (we're
>> forming a path which by definition is not a directory, so
>> the only thing to do with it is feed it to a system call).
>>
>> Let's make the lives of our callers easier by just writing
>> out the internal "/" and the NUL.
>> ...
>
> I think this makes a lot more sense than making the callers have to do this.
The cost of fill function having to do the same thing repeatedly is
negligible, so I am OK with the result, but for fairness, this was
not "make the callers do this extra thing", but was "the caller can
prepare these unchanging parts just once, and the fill function that
is repeatedly run does not have to."
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox