* Re: [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
From: Ramsay Jones @ 2016-09-25 14:07 UTC (permalink / raw)
To: Vegard Nossum, git
Cc: Junio C Hamano, Santi Béjar, Kevin Bracey, Philip Oakley
In-Reply-To: <20160925085511.12515-1-vegard.nossum@oracle.com>
On 25/09/16 09:55, Vegard Nossum wrote:
> I use rev^..rev daily, and I'm surely not the only one. To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev^- a shorthand for that.
>
> The existing syntax rev^! seems like it should do the same, but it
> doesn't really do the right thing for merge commits (it gives only the
> merge itself).
>
> As a natural generalisation, we also accept rev^-n where n excludes the
> nth parent of rev, although this is expected to be generally less useful.
>
> [v2: Use ^- instead of % as suggested by Junio Hamano and use some
> common helper functions for parsing.]
I would place this v2 commentary below the '---' marker (so that it
won't appear in the commit message) ...
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
... here.
> Documentation/revisions.txt | 14 +++++++
> builtin/rev-parse.c | 28 ++++++++++++++
> revision.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
> revision.h | 1 +
> 4 files changed, 134 insertions(+)
>
> diff --git Documentation/revisions.txt Documentation/revisions.txt
> index 4bed5b1..6e33801 100644
> --- Documentation/revisions.txt
> +++ Documentation/revisions.txt
> @@ -281,6 +281,14 @@ is a shorthand for 'HEAD..origin' and asks "What did the origin do since
> I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an
> empty range that is both reachable and unreachable from HEAD.
>
> +Parent Exclusion Notation
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
> +Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> +given. This is typically useful for merge commits where you
> +can just pass '<commit>{caret}-' to get all the commits in the branch
> +that was merged in merge commit '<commit>'.
> +
> Other <rev>{caret} Parent Shorthand Notations
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Two other shorthands exist, particularly useful for merge commits,
> @@ -316,6 +324,10 @@ Revision Range Summary
> <rev2> but exclude those that are reachable from both. When
> either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
>
> +'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::
missing '-' ------------------------------^
> + Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> + given.
> +
> '<rev>{caret}@', e.g. 'HEAD{caret}@'::
> A suffix '{caret}' followed by an at sign is the same as listing
> all parents of '<rev>' (meaning, include anything reachable from
> @@ -339,6 +351,8 @@ spelt out:
> C I J F C
> B..C = ^B C C
> B...C = B ^F C G H D E B C
> + B^- = B^..B
> + = B ^B^1 E I J F B
> C^@ = C^1
> = F I J F
> B^@ = B^1 B^2 B^3
> diff --git builtin/rev-parse.c builtin/rev-parse.c
> index 76cf05e..ad5e6ac 100644
> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
> return 0;
> }
>
> +static int try_parent_exclusion(const char *arg)
> +{
> + int ret = 0;
> + char *to_rev = NULL;
> + char *from_rev = NULL;
> + unsigned char to_sha1[20];
> + unsigned char from_sha1[20];
As Matthieu already mentioned, maybe use 'struct object_id' here.
> +
> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
> + goto out;
> + if (get_sha1_committish(to_rev, to_sha1))
... then 'to_sha1.hash' here, etc ...
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v8 08/11] convert: quote filter names in error messages
From: Jakub Narębski @ 2016-09-25 14:03 UTC (permalink / raw)
To: larsxschneider, git; +Cc: peff, gitster, sbeller, mlbright, tboegi, ramsay
In-Reply-To: <20160920190247.82189-9-larsxschneider@gmail.com>
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> Git filter driver commands with spaces (e.g. `filter.sh foo`) are hard
> to read in error messages. Quote them to improve the readability.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> convert.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
Looks good (those are all sites matching 'error.*%s' in convert.c).
> - return error("cannot fork to run external filter %s", params->cmd);
> + return error("cannot fork to run external filter '%s'", params->cmd);
> - error("cannot feed the input to external filter %s", params->cmd);
> + error("cannot feed the input to external filter '%s'", params->cmd);
> - error("external filter %s failed %d", params->cmd, status);
> + error("external filter '%s' failed %d", params->cmd, status);
> - error("read from external filter %s failed", cmd);
> + error("read from external filter '%s' failed", cmd);
> - error("read from external filter %s failed", cmd);
> + error("read from external filter '%s' failed", cmd);
> - error("external filter %s failed", cmd);
> + error("external filter '%s' failed", cmd);
^ permalink raw reply
* Re: [PATCH v4 3/3] regex: use regexec_buf()
From: Johannes Schindelin @ 2016-09-25 14:01 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Benjamin Kramer, René Scharfe
In-Reply-To: <20160921220310.5wk76qdsjyhvstk4@sigill.intra.peff.net>
Hi Peff,
On Wed, 21 Sep 2016, Jeff King wrote:
> On Wed, Sep 21, 2016 at 08:24:14PM +0200, Johannes Schindelin wrote:
>
> > The new regexec_buf() function operates on buffers with an explicitly
> > specified length, rather than NUL-terminated strings.
> >
> > We need to use this function whenever the buffer we want to pass to
> > regexec() may have been mmap()ed (and is hence not NUL-terminated).
> >
> > Note: the original motivation for this patch was to fix a bug where
> > `git diff -G <regex>` would crash. This patch converts more callers,
> > though, some of which explicitly allocated and constructed
> > NUL-terminated strings (or worse: modified read-only buffers to insert
> > NULs).
>
> Nice. I probably would have split these into their own patch, but I
> think it is OK here.
True. I guess I was a little lazy...
> > @@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,
> > len--;
> > }
> >
> > - line_buffer = xstrndup(line, len); /* make NUL terminated */
> > -
>
> Nice to see this one going away in particular, since it's called quite a
> lot. According to perf, "git log -p" on git.git drops about 1.5 million
> malloc calls (about 9% of the total). And here are best-of-five results
> for that same command:
>
> [before]
> real 0m14.676s
> user 0m13.988s
> sys 0m0.676s
>
> [after]
> real 0m14.394s
> user 0m13.624s
> sys 0m0.760s
>
> Not a _huge_ improvement, but more significant than the run-to-run
> noise.
Oh, that's nice! As you guessed, my aim was not to improve performance,
but it is a pretty side effect...
Thanks for testing!
Dscho
^ permalink raw reply
* Re: [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams
From: Jakub Narębski @ 2016-09-25 13:46 UTC (permalink / raw)
To: Lars Schneider, git
Cc: Jeff King, Junio C Hamano, Stefan Beller, Martin-Louis Bright,
Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160920190247.82189-8-larsxschneider@gmail.com>
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> write_packetized_from_fd() and write_packetized_from_buf() write a
> stream of packets. All content packets use the maximal packet size
> except for the last one. After the last content packet a `flush` control
> packet is written.
>
> read_packetized_to_buf() reads arbitrary sized packets until it detects
> a `flush` packet.
I guess that read_packetized_to_fd(), for completeness, is not needed
for the filter protocol (though it might be useful for the receive
side of send-pack / receive-pack).
Also, should it be read_packetized_to_strbuf()? I guess using strbuf
to read is here because we might need more size to read in full, isn't
it.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> pkt-line.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> pkt-line.h | 7 +++++++
> 2 files changed, 75 insertions(+)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index fc0ac12..a0a8543 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -196,6 +196,47 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
> va_end(args);
> }
>
> +int write_packetized_from_fd(int fd_in, int fd_out)
I wonder if it would be worth it to name parameters in such way that
it is known from the name which one is to be packetized, for example
fd_out_pkt here...
But it might be not worth it; you can get it from the function name.
> +{
> + static char buf[PKTLINE_DATA_MAXLEN];
Static buffer means not thread-safe and not reentrant. It would be
nice to have this information in a comment for this function, but
it is not necessary.
Also, is using static variable better than using global variable
`packet_buffer`? Well, scope for weird interactions is smaller...
Sidenote: we have LARGE_PACKET_MAX (used in previous patch), but
PKTLINE_DATA_MAXLEN not LARGE_PACKET_DATA_MAX.
> + int err = 0;
> + ssize_t bytes_to_write;
> +
> + while (!err) {
> + bytes_to_write = xread(fd_in, buf, sizeof(buf));
> + if (bytes_to_write < 0)
> + return COPY_READ_ERROR;
> + if (bytes_to_write == 0)
> + break;
> + err = packet_write_gently(fd_out, buf, bytes_to_write);
> + }
> + if (!err)
> + err = packet_flush_gently(fd_out);
> + return err;
> +}
Looks good: clean and readable.
Sidenote (probably outside of scope of this patch): what are the
errors that we can get from this function, beside COPY_READ_ERROR
of course?
> +
> +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
> +{
> + static char buf[PKTLINE_DATA_MAXLEN];
> + int err = 0;
> + size_t bytes_written = 0;
> + size_t bytes_to_write;
Those two variables, instead of modifying the values of len and/or src_in,
make code very easy to read.
> +
> + while (!err) {
> + if ((len - bytes_written) > sizeof(buf))
> + bytes_to_write = sizeof(buf);
> + else
> + bytes_to_write = len - bytes_written;
> + if (bytes_to_write == 0)
> + break;
> + err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
> + bytes_written += bytes_to_write;
> + }
> + if (!err)
> + err = packet_flush_gently(fd_out);
> + return err;
> +}
Looks good: clean and readable.
> +
> static int get_packet_data(int fd, char **src_buf, size_t *src_size,
> void *dst, unsigned size, int options)
> {
> @@ -305,3 +346,30 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
> {
> return packet_read_line_generic(-1, src, src_len, dst_len);
> }
> +
> +ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out)
It's a bit strange that the signature of write_packetized_from_buf() is
that different from read_packetized_to_buf(). This includes the return
value: int vs ssize_t. As I have checked, write() and read() both
use ssize_t, while fread() and fwrite() both use size_t.
Perhaps this function should be named read_packetized_to_strbuf()
(err, I asked this already)?
> +{
> + int paket_len;
Possible typo: shouldn't it be called packet_len?
Shouldn't it be initialized to 0?
+ int packet_len = 0;
> + int options = PACKET_READ_GENTLE_ON_EOF;
Why is this even a local variable? It is never changed, and it is
used only in one place; we can inline it.
If it would be needed in subsequent patches, then such information
should be included in the commit message.
> +
> + size_t oldlen = sb_out->len;
> + size_t oldalloc = sb_out->alloc;
Just a nitpick (feel free to ignore): doesn't this looks better:
+ size_t old_len = sb_out->len;
+ size_t old_alloc = sb_out->alloc;
Also perhaps s/old_/orig_/g.
> +
> + for (;;) {
I see that you used the more popular way of coding forever loop:
$ git grep 'for (;;)' -- '*.c' | wc -l
120
$ git grep 'while (1)' -- '*.c' | wc -l
86
> + strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1);
> + paket_len = packet_read(fd_in, NULL, NULL,
> + sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options);
A question (which perhaps was answered during the development of this
patch series): why is this +1 in PKTLINE_DATA_MAXLEN+1 here?
> + if (paket_len <= 0)
> + break;
> + sb_out->len += paket_len;
> + }
> +
> + if (paket_len < 0) {
> + if (oldalloc == 0)
> + strbuf_release(sb_out);
> + else
> + strbuf_setlen(sb_out, oldlen);
A question (maybe I don't understand strbufs): why there is a special
case for oldalloc == 0?
> + return paket_len;
> + }
> + return sb_out->len - oldlen;
> +}
> diff --git a/pkt-line.h b/pkt-line.h
> index 3fa0899..6df8449 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,6 +25,8 @@ void packet_buf_flush(struct strbuf *buf);
> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> int packet_flush_gently(int fd);
> int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> +int write_packetized_from_fd(int fd_in, int fd_out);
> +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
>
> /*
> * Read a packetized line into the buffer, which must be at least size bytes
> @@ -77,6 +79,11 @@ char *packet_read_line(int fd, int *size);
> */
> char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
>
> +/*
> + * Reads a stream of variable sized packets until a flush packet is detected.
> + */
> +ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out);
> +
> #define DEFAULT_PACKET_MAX 1000
> #define LARGE_PACKET_MAX 65520
> extern char packet_buffer[LARGE_PACKET_MAX];
>
^ permalink raw reply
* Re: [PATCH v8 06/11] pkt-line: add packet_write_gently()
From: Jakub Narębski @ 2016-09-25 11:26 UTC (permalink / raw)
To: Lars Schneider, git
Cc: Jeff King, Junio C Hamano, Stefan Beller, Martin-Louis Bright,
Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160920190247.82189-7-larsxschneider@gmail.com>
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> packet_write_fmt_gently() uses format_packet() which lets the caller
> only send string data via "%s". That means it cannot be used for
> arbitrary data that may contain NULs.
>
> Add packet_write_gently() which writes arbitrary data and does not die
> in case of an error. The function is used by other pkt-line functions in
> a subsequent patch.
Nice; obviously needed for sending binary data.
I wonder how send-pack / receive-pack handles sending binary files.
Though this is outside of scope of this patch series, it is something
to think about for later.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> pkt-line.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 19f0271..fc0ac12 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,6 +171,22 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
> return status;
> }
>
> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
I'm not sure what naming convention the rest of Git uses, but isn't
it more like '*data' rather than '*buf' here?
> +{
> + static char packet_write_buffer[LARGE_PACKET_MAX];
I think there should be warning (as a comment before function
declaration, or before function definition), that packet_write_gently()
is not thread-safe (nor reentrant, but the latter does not matter here,
I think).
Thread-safe vs reentrant: http://stackoverflow.com/a/33445858/46058
This is not something terribly important; I guess git code has tons
of functions not marked as thread-unsafe...
> +
> + if (size > sizeof(packet_write_buffer) - 4) {
First, wouldn't the following be more readable:
+ if (size + 4 > LARGE_PACKET_MAX) {
> + return error("packet write failed - data exceeds max packet size");
> + }
Second, CodingGuidelines is against using braces (blocks) for one
line conditionals: "We avoid using braces unnecessarily."
But this is just me nitpicking.
> + packet_trace(buf, size, 1);
> + size += 4;
> + set_packet_header(packet_write_buffer, size);
> + memcpy(packet_write_buffer + 4, buf, size - 4);
> + if (write_in_full(fd_out, packet_write_buffer, size) == size)
Hmmm... in some places we use original size, in others (original) size + 4;
perhaps it would be more readable to add a new local temporary variable
size_t full_size = size + 4;
Or perhaps use 'data_size' and 'packet_size', where 'packet_size = data_size + 4'.
But that might be too chatty for variable names ;-)
> + return 0;
> + return error("packet write failed");
> +}
Compared to previous iterations, where there were two versions
of this function, IIRC sharing no common code: one taking buffer
which had to be with place for packet size info, one with a separate
local buffer for packet size only and using two writes. This
version uses static buffer (thus not thread-safe, I think; and
not reentrant), and memcpy.
Anyway, if reentrant / thread-safe version would be required,
or not doing memcpy turns out to be important with respect to
performance, we can provide with the *_r version:
static int packet_write_gently_r(const int fd_out, const char *data, size_t size,
char *restrict buf)
We can check if 'buf + 4 == data', and if it is, we can skip
memcpy() as an optimization.
This is something for the future, but not very important for
having this patch series accepted.
> +
> void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
> {
> va_list args;
>
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Nazri Ramliy @ 2016-09-25 11:03 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Jeff King, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <CAGZ79kYF6nYL42VQ_mbVzD+2QFLPELe9-8DOQjbSwArM20SCKA@mail.gmail.com>
On Sat, Sep 24, 2016 at 12:34 AM, Stefan Beller <sbeller@google.com> wrote:
> git -C path/to/sub --keep-prefix ls-files
Note that the above can also be written like so (and works
identically), due to the fact that -C can be given multiple times:
git -C path -C to -C sub --keep-prefix ls-files
nazri
^ permalink raw reply
* Re: [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
From: Matthieu Moy @ 2016-09-25 10:25 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Junio C Hamano, Santi Béjar, Kevin Bracey,
Philip Oakley
In-Reply-To: <20160925085511.12515-1-vegard.nossum@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
> Documentation/revisions.txt | 14 +++++++
> builtin/rev-parse.c | 28 ++++++++++++++
> revision.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
> revision.h | 1 +
This would obviously need tests before you can drop the RFC flag.
> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
> return 0;
> }
>
> +static int try_parent_exclusion(const char *arg)
> +{
> + int ret = 0;
> + char *to_rev = NULL;
> + char *from_rev = NULL;
> + unsigned char to_sha1[20];
> + unsigned char from_sha1[20];
I didn't follow closely, but there are patch series by brian m. carlson
to convert these unsigned char array[20] to struct object_id. I guess
adding more arrays is going in the wrong direction. You may want to Cc
brian if unsure.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 1/2] add COPY_ARRAY
From: René Scharfe @ 2016-09-25 8:58 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
In-Reply-To: <20160925074144.44onzv5pub5dxuix@sigill.intra.peff.net>
Am 25.09.2016 um 09:41 schrieb Jeff King:
> On Sun, Sep 25, 2016 at 09:15:42AM +0200, René Scharfe wrote:
>> It checks if the multiplication of size and element count overflows.
>> The inferred size is passed first to st_mult, which allows the division
>> there to be done at compilation time.
>
> I wonder if this actually stops any real overflows. My goal with
> ALLOC_ARRAY, etc, was to catch these at the malloc stage (which is the
> really dangerous part, because we don't want to under-allocate). So the
> first hunk of your patch is:
>
> ALLOC_ARRAY(result, count + 1);
> - memcpy(result, pathspec, count * sizeof(const char *));
> + COPY_ARRAY(result, pathspec, count);
>
> which clearly cannot trigger the st_mult() check, because we would have
> done so in the ALLOC_ARRAY call[1].
>
> Other calls are not so obvious, but in general I would expect the
> allocation step to be doing this check. If we missed one, then it's
> possible that this macro could detect it and prevent a problem. But it
> seems like the wrong time to check. The allocation is buggy, and we'd
> have to just get lucky to be using COPY_ARRAY(). And I don't even mean
> "lucky that we switched to COPY_ARRAY from memcpy for this callsite".
> There are lots of sites that allocate and then fill the array one by
> one, without ever computing the full size again. So allocation is the
> only sensible place to enforce integer overflow.
>
> So I'm not sold on this providing any real integer overflow safety. But
> I do otherwise like it, as it drops the extra "sizeof" which has to
> repeat either the variable name or the type).
Well, yes, checking if the destination object is big enough requires
calculating the size of the source object and thus should include a
check for multiplication overflow already. Note the word "should". :)
If that's the case then a smart compiler could elide the duplicate
check. Keeping the check in COPY_ARRAY reduces the assumptions we have
make about its use as well as any previous checks and doesn't cost much.
René
^ permalink raw reply
* [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
From: Vegard Nossum @ 2016-09-25 8:55 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Santi Béjar, Kevin Bracey, Philip Oakley,
Vegard Nossum
I use rev^..rev daily, and I'm surely not the only one. To save typing
(or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
we can make rev^- a shorthand for that.
The existing syntax rev^! seems like it should do the same, but it
doesn't really do the right thing for merge commits (it gives only the
merge itself).
As a natural generalisation, we also accept rev^-n where n excludes the
nth parent of rev, although this is expected to be generally less useful.
[v2: Use ^- instead of % as suggested by Junio Hamano and use some
common helper functions for parsing.]
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
Documentation/revisions.txt | 14 +++++++
builtin/rev-parse.c | 28 ++++++++++++++
revision.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
revision.h | 1 +
4 files changed, 134 insertions(+)
diff --git Documentation/revisions.txt Documentation/revisions.txt
index 4bed5b1..6e33801 100644
--- Documentation/revisions.txt
+++ Documentation/revisions.txt
@@ -281,6 +281,14 @@ is a shorthand for 'HEAD..origin' and asks "What did the origin do since
I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an
empty range that is both reachable and unreachable from HEAD.
+Parent Exclusion Notation
+~~~~~~~~~~~~~~~~~~~~~~~~~
+The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
+Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
+given. This is typically useful for merge commits where you
+can just pass '<commit>{caret}-' to get all the commits in the branch
+that was merged in merge commit '<commit>'.
+
Other <rev>{caret} Parent Shorthand Notations
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Two other shorthands exist, particularly useful for merge commits,
@@ -316,6 +324,10 @@ Revision Range Summary
<rev2> but exclude those that are reachable from both. When
either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
+'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::
+ Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
+ given.
+
'<rev>{caret}@', e.g. 'HEAD{caret}@'::
A suffix '{caret}' followed by an at sign is the same as listing
all parents of '<rev>' (meaning, include anything reachable from
@@ -339,6 +351,8 @@ spelt out:
C I J F C
B..C = ^B C C
B...C = B ^F C G H D E B C
+ B^- = B^..B
+ = B ^B^1 E I J F B
C^@ = C^1
= F I J F
B^@ = B^1 B^2 B^3
diff --git builtin/rev-parse.c builtin/rev-parse.c
index 76cf05e..ad5e6ac 100644
--- builtin/rev-parse.c
+++ builtin/rev-parse.c
@@ -292,6 +292,32 @@ static int try_difference(const char *arg)
return 0;
}
+static int try_parent_exclusion(const char *arg)
+{
+ int ret = 0;
+ char *to_rev = NULL;
+ char *from_rev = NULL;
+ unsigned char to_sha1[20];
+ unsigned char from_sha1[20];
+
+ if (parse_parent_exclusion(arg, &to_rev, &from_rev))
+ goto out;
+ if (get_sha1_committish(to_rev, to_sha1))
+ goto out;
+ if (get_sha1_committish(from_rev, from_sha1))
+ goto out;
+
+ show_rev(NORMAL, to_sha1, to_rev);
+ show_rev(REVERSED, from_sha1, from_rev);
+
+ ret = 1;
+
+out:
+ free(to_rev);
+ free(from_rev);
+ return ret;
+}
+
static int try_parent_shorthands(const char *arg)
{
char *dotdot;
@@ -839,6 +865,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
/* Not a flag argument */
if (try_difference(arg))
continue;
+ if (try_parent_exclusion(arg))
+ continue;
if (try_parent_shorthands(arg))
continue;
name = arg;
diff --git revision.c revision.c
index 969b3d1..0480f19 100644
--- revision.c
+++ revision.c
@@ -1419,6 +1419,93 @@ static void prepare_show_merge(struct rev_info *revs)
revs->limited = 1;
}
+/*
+ * If 'arg' is on the form '<rev>^-{<n>}', then return 0 and
+ * '*to_rev' and '*from_rev' will contain '<rev>' and '<rev>^<n>',
+ * respectively.
+ */
+int parse_parent_exclusion(const char *arg, char **to_rev, char **from_rev)
+{
+ char *caret;
+ unsigned int n = 1;
+
+ /*
+ * <rev>^-{<n>} is shorthand for <rev>^<n>..<rev>, with <n> = 1 if
+ * not given. This is typically used for merge commits where you
+ * can just pass '<merge>^-' and it will show you all the commits in
+ * the branch that was merged.
+ */
+
+ if (!(caret = strstr(arg, "^-")))
+ return 1;
+ if (caret[2]) {
+ char *end;
+ n = strtoul(&caret[2], &end, 10);
+ if (*end != '\0')
+ return 1;
+ }
+ *to_rev = xstrndup(arg, caret - arg);
+ *from_rev = xstrfmt("%s^%u", *to_rev, n);
+ return 0;
+}
+
+static int handle_parent_exclusion(const char *arg, struct rev_info *revs, int flags)
+{
+ int ret = 1;
+ char *to_rev = NULL;
+ char *from_rev = NULL;
+ unsigned char to_sha1[20];
+ unsigned char from_sha1[20];
+
+ struct object *a_obj, *b_obj;
+ unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
+ unsigned int a_flags;
+
+ /*
+ * <rev>^-{<n>} is shorthand for <rev>^<n>..<rev>, with <n> = 1 if
+ * not given. This is typically used for merge commits where you
+ * can just pass <merge>^- and it will show you all the commits in
+ * the branches that were merged.
+ */
+
+ if (parse_parent_exclusion(arg, &to_rev, &from_rev))
+ goto out;
+
+ if (get_sha1_committish(to_rev, to_sha1)) {
+ if (revs->ignore_missing)
+ goto out;
+ die("Unknown revision %s", to_rev);
+ }
+
+ if (get_sha1_committish(from_rev, from_sha1)) {
+ if (revs->ignore_missing)
+ goto out;
+ die("Unknown revision %s", from_rev);
+ }
+
+ a_obj = parse_object(from_sha1);
+ b_obj = parse_object(to_sha1);
+ if (!a_obj || !b_obj) {
+ if (revs->ignore_missing)
+ goto out;
+ die("Invalid revision range %s", arg);
+ }
+
+ a_flags = flags_exclude;
+ a_obj->flags |= a_flags;
+ b_obj->flags |= flags;
+ add_rev_cmdline(revs, a_obj, from_rev, REV_CMD_LEFT, a_flags);
+ add_pending_object(revs, a_obj, from_rev);
+ add_rev_cmdline(revs, b_obj, to_rev, REV_CMD_RIGHT, flags);
+ add_pending_object(revs, b_obj, to_rev);
+
+ ret = 0;
+out:
+ free(to_rev);
+ free(from_rev);
+ return ret;
+}
+
int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
{
struct object_context oc;
@@ -1519,6 +1606,10 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
}
*dotdot = '.';
}
+
+ if (!handle_parent_exclusion(arg, revs, flags))
+ return 0;
+
dotdot = strstr(arg, "^@");
if (dotdot && !dotdot[2]) {
*dotdot = 0;
diff --git revision.h revision.h
index 9fac1a6..ca5bebc 100644
--- revision.h
+++ revision.h
@@ -243,6 +243,7 @@ extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,
extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
const struct option *options,
const char * const usagestr[]);
+extern int parse_parent_exclusion(const char *arg, char **to_rev, char **from_rev);
#define REVARG_CANNOT_BE_FILENAME 01
#define REVARG_COMMITTISH 02
extern int handle_revision_arg(const char *arg, struct rev_info *revs,
--
2.10.0.rc0.1.g07c9292
^ permalink raw reply related
* Re: [PATCH 1/2] add COPY_ARRAY
From: Jeff King @ 2016-09-25 7:41 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <6f402d35-b483-7552-2fb2-a5350112b8a6@web.de>
On Sun, Sep 25, 2016 at 09:15:42AM +0200, René Scharfe wrote:
> Add COPY_ARRAY, a safe and convenient helper for copying arrays,
> complementing ALLOC_ARRAY and REALLOC_ARRAY. Users just specify source,
> destination and the number of elements; the size of an element is
> inferred automatically.
Seems like a fairly readable construct to have.
> It checks if the multiplication of size and element count overflows.
> The inferred size is passed first to st_mult, which allows the division
> there to be done at compilation time.
I wonder if this actually stops any real overflows. My goal with
ALLOC_ARRAY, etc, was to catch these at the malloc stage (which is the
really dangerous part, because we don't want to under-allocate). So the
first hunk of your patch is:
ALLOC_ARRAY(result, count + 1);
- memcpy(result, pathspec, count * sizeof(const char *));
+ COPY_ARRAY(result, pathspec, count);
which clearly cannot trigger the st_mult() check, because we would have
done so in the ALLOC_ARRAY call[1].
Other calls are not so obvious, but in general I would expect the
allocation step to be doing this check. If we missed one, then it's
possible that this macro could detect it and prevent a problem. But it
seems like the wrong time to check. The allocation is buggy, and we'd
have to just get lucky to be using COPY_ARRAY(). And I don't even mean
"lucky that we switched to COPY_ARRAY from memcpy for this callsite".
There are lots of sites that allocate and then fill the array one by
one, without ever computing the full size again. So allocation is the
only sensible place to enforce integer overflow.
So I'm not sold on this providing any real integer overflow safety. But
I do otherwise like it, as it drops the extra "sizeof" which has to
repeat either the variable name or the type).
-Peff
[1] Actually, this particular example probably should be using
st_add(count, 1), though it's likely not a problem in practice
("count" is an int, so you cannot easily overflow it back to 0 by
incrementing 1, though of course overflowing it at all is undefined
behavior).
^ permalink raw reply
* Re: [PATCH 6/7] use COPY_ARRAY
From: René Scharfe @ 2016-09-25 7:26 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <869cff97-9e5f-ec17-6b64-bd1e4d9d1947@web.de>
Ha, can't count. It should be [PATCH 2/2] of course.
René
^ permalink raw reply
* [PATCH 6/7] use COPY_ARRAY
From: René Scharfe @ 2016-09-25 7:24 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <6f402d35-b483-7552-2fb2-a5350112b8a6@web.de>
Add a semantic patch for converting certain calls of memcpy(3) to
COPY_ARRAY() and apply that transformation to the code base. The result
is
shorter and safer code. For now only consider calls where source and
destination have the same type, or in other words: easy cases.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
builtin/mv.c | 2 +-
commit.c | 2 +-
contrib/coccinelle/array.cocci | 26 ++++++++++++++++++++++++++
pack-revindex.c | 2 +-
pathspec.c | 3 +--
split-index.c | 6 ++----
6 files changed, 32 insertions(+), 9 deletions(-)
create mode 100644 contrib/coccinelle/array.cocci
diff --git a/builtin/mv.c b/builtin/mv.c
index 446a316..2f43877 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -26,7 +26,7 @@ static const char **internal_copy_pathspec(const char *prefix,
int i;
const char **result;
ALLOC_ARRAY(result, count + 1);
- memcpy(result, pathspec, count * sizeof(const char *));
+ COPY_ARRAY(result, pathspec, count);
result[count] = NULL;
for (i = 0; i < count; i++) {
int length = strlen(result[i]);
diff --git a/commit.c b/commit.c
index ba6dee3..aada266 100644
--- a/commit.c
+++ b/commit.c
@@ -931,7 +931,7 @@ static int remove_redundant(struct commit **array, int cnt)
}
/* Now collect the result */
- memcpy(work, array, sizeof(*array) * cnt);
+ COPY_ARRAY(work, array, cnt);
for (i = filled = 0; i < cnt; i++)
if (!redundant[i])
array[filled++] = work[i];
diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
new file mode 100644
index 0000000..2d7f25d
--- /dev/null
+++ b/contrib/coccinelle/array.cocci
@@ -0,0 +1,26 @@
+@@
+type T;
+T *dst;
+T *src;
+expression n;
+@@
+- memcpy(dst, src, n * sizeof(*dst));
++ COPY_ARRAY(dst, src, n);
+
+@@
+type T;
+T *dst;
+T *src;
+expression n;
+@@
+- memcpy(dst, src, n * sizeof(*src));
++ COPY_ARRAY(dst, src, n);
+
+@@
+type T;
+T *dst;
+T *src;
+expression n;
+@@
+- memcpy(dst, src, n * sizeof(T));
++ COPY_ARRAY(dst, src, n);
diff --git a/pack-revindex.c b/pack-revindex.c
index 96d51c3..6bc7c94 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -107,7 +107,7 @@ static void sort_revindex(struct revindex_entry *entries, unsigned n, off_t max)
* we have to move it back from the temporary storage.
*/
if (from != entries)
- memcpy(entries, tmp, n * sizeof(*entries));
+ COPY_ARRAY(entries, tmp, n);
free(tmp);
free(pos);
diff --git a/pathspec.c b/pathspec.c
index 24e0dd5..49a5360 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -485,8 +485,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
{
*dst = *src;
ALLOC_ARRAY(dst->items, dst->nr);
- memcpy(dst->items, src->items,
- sizeof(struct pathspec_item) * dst->nr);
+ COPY_ARRAY(dst->items, src->items, dst->nr);
}
void clear_pathspec(struct pathspec *pathspec)
diff --git a/split-index.c b/split-index.c
index 3c75d4b..35da553 100644
--- a/split-index.c
+++ b/split-index.c
@@ -83,8 +83,7 @@ void move_cache_to_base_index(struct index_state *istate)
si->base->timestamp = istate->timestamp;
ALLOC_GROW(si->base->cache, istate->cache_nr, si->base->cache_alloc);
si->base->cache_nr = istate->cache_nr;
- memcpy(si->base->cache, istate->cache,
- sizeof(*istate->cache) * istate->cache_nr);
+ COPY_ARRAY(si->base->cache, istate->cache, istate->cache_nr);
mark_base_index_entries(si->base);
for (i = 0; i < si->base->cache_nr; i++)
si->base->cache[i]->ce_flags &= ~CE_UPDATE_IN_BASE;
@@ -141,8 +140,7 @@ void merge_base_index(struct index_state *istate)
istate->cache = NULL;
istate->cache_alloc = 0;
ALLOC_GROW(istate->cache, istate->cache_nr, istate->cache_alloc);
- memcpy(istate->cache, si->base->cache,
- sizeof(*istate->cache) * istate->cache_nr);
+ COPY_ARRAY(istate->cache, si->base->cache, istate->cache_nr);
si->nr_deletions = 0;
si->nr_replacements = 0;
--
2.10.0
^ permalink raw reply related
* Re: [PATCH 0/3] recursive support for ls-files
From: Jeff King @ 2016-09-25 7:17 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474676014-134568-1-git-send-email-bmwill@google.com>
On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:
> After looking at the feedback I rerolled a few things, in particular the
> --submodule_prefix option that existed to give a submodule context about where
> it had been invoked from. People didn't seem to like the idea of exposing this
> to the users (yet anyways) so I removed it as an option and instead have it
> being passed to a child process via an environment variable
> GIT_INTERNAL_SUBMODULE_PREFIX. This way we don't have to support anything to
> external users at the moment.
I think we can still have it as a command-line argument and declare it
internal. It's not like environment variables cannot also be set by our
callers. :)
I don't mind it as an environment variable, though. In some ways it
makes things easier. I just think "internal versus external" and the
exact implementation are orthogonal.
> Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
> King.
Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd
try not to have a broken state in the history. It's less important in
this case, because the breakage is not a regression
(--recurse-submodules is a new feature, so you could consider it "not
working" until the 3rd patch). But I think it's still a good rule to
follow, because it makes the commits easier to review, look at later,
etc.
For that matter, I do not understand why options like "-s" get enabled
in patch 3. I do not mind them starting as disabled in patch 2, but it
seems like "pass along some known-safe options" should be its own patch
somewhere between patches 2 and 3.
There are some other options that are ignored (neither disabled nor
passed along to children). Most of them are related to exclusions, which
I _think_ are safe to ignore (they do not do anything interesting unless
you specify "-o", which is explicitly disabled). I'm not sure about
--with-tree, though (or what it would even mean in the context of
recursing).
-Peff
^ permalink raw reply
* [PATCH 1/2] add COPY_ARRAY
From: René Scharfe @ 2016-09-25 7:15 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jeff King
Add COPY_ARRAY, a safe and convenient helper for copying arrays,
complementing ALLOC_ARRAY and REALLOC_ARRAY. Users just specify source,
destination and the number of elements; the size of an element is
inferred automatically.
It checks if the multiplication of size and element count overflows.
The inferred size is passed first to st_mult, which allows the division
there to be done at compilation time.
As a basic type safety check it makes sure the sizes of source and
destination elements are the same. That's evaluated at compilation time
as well.
COPY_ARRAY is safe to use with NULL as source pointer iff 0 elements are
to be copied. That convention is used in some cases for initializing
arrays. Raw memcpy(3) does not support it -- compilers are allowed to
assume that only valid pointers are passed to it and can optimize away
NULL checks after such a call.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
git-compat-util.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 37cce07..91775ce 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -801,6 +801,14 @@ extern FILE *fopen_for_writing(const char *path);
#define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
+#define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \
+ BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))
+static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
+{
+ if (n)
+ memcpy(dst, src, st_mult(size, n));
+}
+
/*
* These functions help you allocate structs with flex arrays, and copy
* the data directly into the array. For example, if you had:
--
2.10.0
^ permalink raw reply related
* Request: Extra case for %G? format
From: Alex @ 2016-09-25 6:05 UTC (permalink / raw)
To: git
Hello all,
Could the %G? format differentiate between an unsigned commit and a
signed commit that you're missing a public key for?
If `git show --format=%GG --no-patch <commit>' produces an output like
the following:
gpg: Signature made <date> using RSA key ID <id>
gpg: Can't check signature: public key not found
Then currently %G? results in `N', the same as an unsigned commit.
In this case, could %G? please result in a new character? Perhaps `M'
for "missing public key"?
^ permalink raw reply
* [PATCH v3 5/5] init: kill git_link variable
From: Nguyễn Thái Ngọc Duy @ 2016-09-25 3:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, git, max.nordlund, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160925031440.10435-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/init-db.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 8069cd2..37e318b 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -22,7 +22,6 @@
static int init_is_bare_repository = 0;
static int init_shared_repository = -1;
static const char *init_db_template_dir;
-static const char *git_link;
static void copy_templates_1(struct strbuf *path, struct strbuf *template,
DIR *dir)
@@ -312,7 +311,7 @@ static void create_object_directory(void)
strbuf_release(&path);
}
-static void separate_git_dir(const char *git_dir)
+static void separate_git_dir(const char *git_dir, const char *git_link)
{
struct stat st;
@@ -349,22 +348,15 @@ int init_db(const char *git_dir, const char *real_git_dir,
if (!exist_ok && !stat(real_git_dir, &st))
die(_("%s already exists"), real_git_dir);
- /*
- * make sure symlinks are resolved because we'll be
- * moving the target repo later on in separate_git_dir()
- */
- git_link = xstrdup(real_path(git_dir));
set_git_dir(real_path(real_git_dir));
+ git_dir = get_git_dir();
+ separate_git_dir(git_dir, original_git_dir);
}
else {
set_git_dir(real_path(git_dir));
- git_link = NULL;
+ git_dir = get_git_dir();
}
startup_info->have_repository = 1;
- git_dir = get_git_dir();
-
- if (git_link)
- separate_git_dir(git_dir);
safe_create_dir(git_dir, 0);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v3 4/5] init: do not set unnecessary core.worktree
From: Nguyễn Thái Ngọc Duy @ 2016-09-25 3:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, git, max.nordlund, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160925031440.10435-1-pclouds@gmail.com>
The function needs_work_tree_config() that is called from
create_default_files() is supposed to be fed the path to ".git" that
looks as if it is at the top of the working tree, and decide if that
location matches the actual worktree being used. This comparison allows
"git init" to decide if core.worktree needs to be recorded in the
working tree.
In the current code, however, we feed the return value from
get_git_dir(), which can be totally different from what the function
expects when "gitdir" file is involved. Instead of giving the path to
the ".git" at the top of the working tree, we end up feeding the actual
path that the file points at.
This original location of ".git" however is only known to init_db().
Make init_db() save it and have it passed to create_default_files() as a
new parameter, which passes the correct location down to
needs_work_tree_config() to fix this.
Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/init-db.c | 9 ++++++---
t/t0001-init.sh | 2 ++
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 68c1ae3..8069cd2 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -171,7 +171,8 @@ static int needs_work_tree_config(const char *git_dir, const char *work_tree)
return 1;
}
-static int create_default_files(const char *template_path)
+static int create_default_files(const char *template_path,
+ const char *original_git_dir)
{
struct stat st1;
struct strbuf buf = STRBUF_INIT;
@@ -263,7 +264,7 @@ static int create_default_files(const char *template_path)
/* allow template config file to override the default */
if (log_all_ref_updates == -1)
git_config_set("core.logallrefupdates", "true");
- if (needs_work_tree_config(get_git_dir(), work_tree))
+ if (needs_work_tree_config(original_git_dir, work_tree))
git_config_set("core.worktree", work_tree);
}
@@ -337,6 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
{
int reinit;
int exist_ok = flags & INIT_DB_EXIST_OK;
+ char *original_git_dir = xstrdup(real_path(git_dir));
if (real_git_dir) {
struct stat st;
@@ -375,7 +377,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
*/
check_repository_format();
- reinit = create_default_files(template_dir);
+ reinit = create_default_files(template_dir, original_git_dir);
create_object_directory();
@@ -412,6 +414,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
git_dir, len && git_dir[len-1] != '/' ? "/" : "");
}
+ free(original_git_dir);
return 0;
}
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 488564b..b8fc588 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -400,9 +400,11 @@ test_expect_success 're-init from a linked worktree' '
test_commit first &&
git worktree add ../linked-worktree &&
mv .git/info/exclude expected-exclude &&
+ cp .git/config expected-config &&
find .git/worktrees -print | sort >expected &&
git -C ../linked-worktree init &&
test_cmp expected-exclude .git/info/exclude &&
+ test_cmp expected-config .git/config &&
find .git/worktrees -print | sort >actual &&
test_cmp expected actual
)
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v3 3/5] init: kill set_git_dir_init()
From: Nguyễn Thái Ngọc Duy @ 2016-09-25 3:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, git, max.nordlund, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160925031440.10435-1-pclouds@gmail.com>
This is a pure code move, necessary to kill the global variable git_link
later (and also helps a bit in the next patch).
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/init-db.c | 50 +++++++++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 29 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 5cb05d9..68c1ae3 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -311,34 +311,6 @@ static void create_object_directory(void)
strbuf_release(&path);
}
-static int set_git_dir_init(const char *git_dir,
- const char *real_git_dir,
- int exist_ok)
-{
- if (real_git_dir) {
- struct stat st;
-
- if (!exist_ok && !stat(git_dir, &st))
- die(_("%s already exists"), git_dir);
-
- if (!exist_ok && !stat(real_git_dir, &st))
- die(_("%s already exists"), real_git_dir);
-
- /*
- * make sure symlinks are resolved because we'll be
- * moving the target repo later on in separate_git_dir()
- */
- git_link = xstrdup(real_path(git_dir));
- set_git_dir(real_path(real_git_dir));
- }
- else {
- set_git_dir(real_path(git_dir));
- git_link = NULL;
- }
- startup_info->have_repository = 1;
- return 0;
-}
-
static void separate_git_dir(const char *git_dir)
{
struct stat st;
@@ -364,9 +336,29 @@ int init_db(const char *git_dir, const char *real_git_dir,
const char *template_dir, unsigned int flags)
{
int reinit;
+ int exist_ok = flags & INIT_DB_EXIST_OK;
- set_git_dir_init(git_dir, real_git_dir, flags & INIT_DB_EXIST_OK);
+ if (real_git_dir) {
+ struct stat st;
+ if (!exist_ok && !stat(git_dir, &st))
+ die(_("%s already exists"), git_dir);
+
+ if (!exist_ok && !stat(real_git_dir, &st))
+ die(_("%s already exists"), real_git_dir);
+
+ /*
+ * make sure symlinks are resolved because we'll be
+ * moving the target repo later on in separate_git_dir()
+ */
+ git_link = xstrdup(real_path(git_dir));
+ set_git_dir(real_path(real_git_dir));
+ }
+ else {
+ set_git_dir(real_path(git_dir));
+ git_link = NULL;
+ }
+ startup_info->have_repository = 1;
git_dir = get_git_dir();
if (git_link)
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v3 2/5] init: call set_git_dir_init() from within init_db()
From: Nguyễn Thái Ngọc Duy @ 2016-09-25 3:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, git, max.nordlund, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160925031440.10435-1-pclouds@gmail.com>
The next commit requires that set_git_dir_init() must be called before
init_db(). Let's make sure nobody can do otherwise.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/clone.c | 15 +++++++--------
builtin/init-db.c | 18 +++++++++++-------
cache.h | 5 +++--
3 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 6616392..29b1832 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -928,23 +928,22 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
set_git_work_tree(work_tree);
}
- junk_git_dir = git_dir;
+ junk_git_dir = real_git_dir ? real_git_dir : git_dir;
if (safe_create_leading_directories_const(git_dir) < 0)
die(_("could not create leading directories of '%s'"), git_dir);
- set_git_dir_init(git_dir, real_git_dir, 0);
- if (real_git_dir) {
- git_dir = real_git_dir;
- junk_git_dir = real_git_dir;
- }
-
if (0 <= option_verbosity) {
if (option_bare)
fprintf(stderr, _("Cloning into bare repository '%s'...\n"), dir);
else
fprintf(stderr, _("Cloning into '%s'...\n"), dir);
}
- init_db(option_template, INIT_DB_QUIET);
+
+ init_db(git_dir, real_git_dir, option_template, INIT_DB_QUIET);
+
+ if (real_git_dir)
+ git_dir = real_git_dir;
+
write_config(&option_config);
git_config(git_default_config, NULL);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index d5d7558..5cb05d9 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -311,8 +311,9 @@ static void create_object_directory(void)
strbuf_release(&path);
}
-int set_git_dir_init(const char *git_dir, const char *real_git_dir,
- int exist_ok)
+static int set_git_dir_init(const char *git_dir,
+ const char *real_git_dir,
+ int exist_ok)
{
if (real_git_dir) {
struct stat st;
@@ -359,10 +360,14 @@ static void separate_git_dir(const char *git_dir)
write_file(git_link, "gitdir: %s", git_dir);
}
-int init_db(const char *template_dir, unsigned int flags)
+int init_db(const char *git_dir, const char *real_git_dir,
+ const char *template_dir, unsigned int flags)
{
int reinit;
- const char *git_dir = get_git_dir();
+
+ set_git_dir_init(git_dir, real_git_dir, flags & INIT_DB_EXIST_OK);
+
+ git_dir = get_git_dir();
if (git_link)
separate_git_dir(git_dir);
@@ -582,7 +587,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
set_git_work_tree(work_tree);
}
- set_git_dir_init(git_dir, real_git_dir, 1);
-
- return init_db(template_dir, flags);
+ flags |= INIT_DB_EXIST_OK;
+ return init_db(git_dir, real_git_dir, template_dir, flags);
}
diff --git a/cache.h b/cache.h
index b2d77f3..7fc875f 100644
--- a/cache.h
+++ b/cache.h
@@ -525,9 +525,10 @@ extern void verify_non_filename(const char *prefix, const char *name);
extern int path_inside_repo(const char *prefix, const char *path);
#define INIT_DB_QUIET 0x0001
+#define INIT_DB_EXIST_OK 0x0002
-extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);
-extern int init_db(const char *template_dir, unsigned int flags);
+extern int init_db(const char *git_dir, const char *real_git_dir,
+ const char *template_dir, unsigned int flags);
extern void sanitize_stdfds(void);
extern int daemonize(void);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v3 1/5] init: correct re-initialization from a linked worktree
From: Nguyễn Thái Ngọc Duy @ 2016-09-25 3:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, git, max.nordlund, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160925031329.GA9124@ash>
When 'git init' is called from a linked worktree, we treat '.git'
dir (which is $GIT_COMMON_DIR/worktrees/something) as the main
'.git' (i.e. $GIT_COMMON_DIR) and populate the whole repository skeleton
in there. It does not harm anything (*) but it is still wrong.
Since 'git init' calls set_git_dir() at preparation time, which
indirectly calls get_common_dir() and correctly detects multiple
worktree setup, all git_path_buf() calls in create_default_files() will
return correct paths in both single and multiple worktree setups. The
only thing left is copy_templates(), which targets $GIT_DIR, not
$GIT_COMMON_DIR.
Fix that with get_git_common_dir(). This function will return $GIT_DIR
in single-worktree setup, so we don't have to make a special case for
multiple-worktree here.
(*) It does in fact, thanks to another bug. More on that later.
Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/init-db.c | 2 +-
t/t0001-init.sh | 15 +++++++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index cc09fca..d5d7558 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -138,7 +138,7 @@ static void copy_templates(const char *template_dir)
goto close_free_return;
}
- strbuf_addstr(&path, get_git_dir());
+ strbuf_addstr(&path, get_git_common_dir());
strbuf_complete(&path, '/');
copy_templates_1(&path, &template_path, dir);
close_free_return:
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 8ffbbea..488564b 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -393,4 +393,19 @@ test_expect_success 'remote init from does not use config from cwd' '
test_cmp expect actual
'
+test_expect_success 're-init from a linked worktree' '
+ git init main-worktree &&
+ (
+ cd main-worktree &&
+ test_commit first &&
+ git worktree add ../linked-worktree &&
+ mv .git/info/exclude expected-exclude &&
+ find .git/worktrees -print | sort >expected &&
+ git -C ../linked-worktree init &&
+ test_cmp expected-exclude .git/info/exclude &&
+ find .git/worktrees -print | sort >actual &&
+ test_cmp expected actual
+ )
+'
+
test_done
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one
From: Duy Nguyen @ 2016-09-25 3:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, git, max.nordlund
In-Reply-To: <xmqq8tuhyvoa.fsf@gitster.mtv.corp.google.com>
On Sat, Sep 24, 2016 at 11:55:33AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I think this 4/3 is not quite enough to fix the damage to the code
> > caused by 2/3.
> > ...
> > after 4/3 is applied, we should be able to remove the global
> > variable 2/3 introduced, make init_db() receive that information as
> > the return value of set_git_dir_init(), and pass that as a parameter
> > to create_default_files().
>
> That would look something like this squashed into 4/3, I think. I
> am not sure if a commit that squashes 2/3, 3/3, 4/3 and this update
> together is harder to understand than keeping 2/3, 3/3 and a fixed
> 4/3 separate, though. The end result looks much better structured
> than before 2/3 is applied to my quick scan-through of the code.
> ...
How about this?
[1/5] init: correct re-initialization from a linked worktree
[2/5] init: call set_git_dir_init() from within init_db()
[3/5] init: kill set_git_dir_init()
[4/5] init: do not set unnecessary core.worktree
[5/5] init: kill git_link variable
I went a bit further, merging set_git_dir_init() back to init_db() so
I can kill "git_link" variable cleanly in 5/5. 3/5 does make init_db()
a bit longer, but not to the alarming level yet. By 4/5,
set_git_dir_init() is gone, so init_db() just needs to save and pass
original_git_dir down to needs_work_tree_config().
--
Duy
^ permalink raw reply
* Re: [PATCH v8 05/11] pkt-line: add packet_flush_gently()
From: Jakub Narębski @ 2016-09-24 22:56 UTC (permalink / raw)
To: Lars Schneider, git
Cc: Jeff King, Junio C Hamano, Stefan Beller, Martin-Louis Bright,
Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160920190247.82189-6-larsxschneider@gmail.com>
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> packet_flush() would die in case of a write error even though for some
> callers an error would be acceptable. Add packet_flush_gently() which
> writes a pkt-line flush packet like packet_flush() but does not die in
> case of an error. The function is used in a subsequent patch.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Looks good.
I guess the difference in treatment from packet_write_fmt_gently() in
the previous patch is that there isn't anything to extract to form
a common code function... I have skipped a few iterations of this series.
> ---
> pkt-line.c | 8 ++++++++
> pkt-line.h | 1 +
> 2 files changed, 9 insertions(+)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 3b465fd..19f0271 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -91,6 +91,14 @@ void packet_flush(int fd)
> write_or_die(fd, "0000", 4);
> }
>
> +int packet_flush_gently(int fd)
> +{
> + packet_trace("0000", 4, 1);
> + if (write_in_full(fd, "0000", 4) == 4)
> + return 0;
> + return error("flush packet write failed");
> +}
> +
> void packet_buf_flush(struct strbuf *buf)
> {
> packet_trace("0000", 4, 1);
> diff --git a/pkt-line.h b/pkt-line.h
> index 3caea77..3fa0899 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -23,6 +23,7 @@ void packet_flush(int fd);
> void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> void packet_buf_flush(struct strbuf *buf);
> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> +int packet_flush_gently(int fd);
> int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
>
> /*
>
^ permalink raw reply
* Re: [PATCH v3 2/2] gitweb: use highlight's shebang detection
From: Ian Kelling @ 2016-09-24 22:35 UTC (permalink / raw)
To: Jakub Narębski, git
In-Reply-To: <946807ff-1570-2d81-1026-06529164f8ef@gmail.com>
On Sat, Sep 24, 2016, at 09:21 AM, Jakub Narębski wrote:
> W dniu 24.09.2016 o 00:15, Jakub Narębski pisze:
>
> Sidenote: this way of benchmarking of gitweb falls between two ways of
> doing a benchmark.
>
> The first method is to simply run gitweb as a standalone script, passing
> its parameters in CGI environment variables; just like the test suite
> does it. You would 'time' / 'times' it a few times, drop outliers, and
> take average or a median. With this method you don't even need to set
> up a web server.
>
> The second is to use a specialized program to benchmark the server-side
> of a web page, for example 'ab' (ApacheBench), httperf, curl-loader
> or JMeter. The first one is usually distributed together with Apache
> web server, so you probably have it installed already. Those tools
> provide timing statistics.
Good to know. Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/2] gitweb: use highlight's shebang detection
From: Ian Kelling @ 2016-09-24 22:34 UTC (permalink / raw)
To: Jakub Narębski, git
In-Reply-To: <2a4c3efb-2145-b699-c980-3079f165a6e1@gmail.com>
On Fri, Sep 23, 2016, at 03:15 PM, Jakub Narębski wrote:
> W dniu 23.09.2016 o 11:08, Ian Kelling napisał:
>
> > The "highlight" binary can, in some cases, determine the language type
> > by the means of file contents, for example the shebang in the first line
> > for some scripting languages. Make use of this autodetection for files
> > which syntax is not known by gitweb. In that case, pass the blob
> > contents to "highlight --force"; the parameter is needed to make it
> > always generate HTML output (which includes HTML-escaping).
>
> Right.
>
> >
> > Although we now run highlight on files which do not end up highlighted,
> > performance is virtually unaffected because when we call highlight, we
> > also call sanitize() instead of esc_html(), which is significantly
> > slower.
>
> This paragraph is a bit unclear, for example it is not obvious what
> "..., which is significantly slower" refers to: sanitize() or esc_html().
>
> I think it would be better to write:
>
> Although we now run highlight on files which do not end up highlighted,
> performance is virtually unaffected because when we call highlight, it
> is used for escaping HTML. In the case that highlight is used, gitweb
> calls sanitize() instead of esc_html(), and the latter is significantly
> slower (it does more, being roughly a superset of sanitize()).
Agree. Done in v4.
>
> > After curling blob view of unhighlighted large and small text
> > files of perl code and license text 100 times each on a local
> > Apache/2.4.23 (Debian) instance, it's logs indicate +-1% difference in
> > request time for all file types.
>
> Also, "curling" is not the word I would like to see. I would say:
>
> Simple benchmark comparing performance of 'blob' view of files without
> syntax highlighting in gitweb before and after this change indicates
> ±1% difference in request time for all file types. Benchmark was
> performed on local instance on Debian, using Apache/2.4.23 web server
> and CGI/PSGI/FCGI/mod_perl.
>
> ^^^^^^^^^^^^^^^^^^^^^^--- select one
>
> Or something like that; I'm not sure how detailed this should be.
> But it is nice to have such benchmark in the commit message.
Sounds good. Used it in v4.
>
> Anyway I think that adding yet another configuration toggle for selecting
> whether to use "highlight" syntax autodetection or not would be just an
> unnecessary complication.
>
> Note that the performance loss might be quite higher on MS Windows, with
> its higher cost of fork. But then they probably do not configure
> server-side highligher anyway.
>
> >
> > Document the feature and improve syntax highlight documentation, add
> > test to ensure gitweb doesn't crash when language detection is used.
>
> Good.
>
> >
> > Signed-off-by: Ian Kelling <ian@iankelling.org>
> > ---
> > Documentation/gitweb.conf.txt | 21 ++++++++++++++-------
> > gitweb/gitweb.perl | 10 +++++-----
> > t/t9500-gitweb-standalone-no-errors.sh | 8 ++++++++
> > 3 files changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> > index a79e350..e632089 100644
> > --- a/Documentation/gitweb.conf.txt
> > +++ b/Documentation/gitweb.conf.txt
> > @@ -246,13 +246,20 @@ $highlight_bin::
>
> We should probably say what does it mean to be "highlight"[1] compatible,
> but it is outside of scope for this patch, and I think also out of scope
> of this series.
>
> > Note that 'highlight' feature must be set for gitweb to actually
> > use syntax highlighting.
> > +
> > -*NOTE*: if you want to add support for new file type (supported by
> > -"highlight" but not used by gitweb), you need to modify `%highlight_ext`
> > -or `%highlight_basename`, depending on whether you detect type of file
> > -based on extension (for example "sh") or on its basename (for example
> > -"Makefile"). The keys of these hashes are extension and basename,
> > -respectively, and value for given key is name of syntax to be passed via
> > -`--syntax <syntax>` to highlighter.
> > +*NOTE*: for a file to be highlighted, its syntax type must be detected
> > +and that syntax must be supported by "highlight". The default syntax
> > +detection is minimal, and there are many supported syntax types with no
> > +detection by default. There are three options for adding syntax
> > +detection. The first and second priority are `%highlight_basename` and
> > +`%highlight_ext`, which detect based on basename (the full filename, for
> > +example "Makefile") and extension (for example "sh"). The keys of these
> > +hashes are the basename and extension, respectively, and the value for a
> > +given key is the name of the syntax to be passed via `--syntax <syntax>`
> > +to "highlight". The last priority is the "highlight" configuration of
> > +`Shebang` regular expressions to detect the language based on the first
> > +line in the file, (for example, matching the line "#!/bin/bash"). See
> > +the highlight documentation and the default config at
> > +/etc/highlight/filetypes.conf for more details.
>
> All right. I guess /etc/highlight/filetypes.conf is the standard
> location?
I think so. I checked packages from homebrew, fedora, suse, debian to
confirm.
>
> > +
> > For example if repositories you are hosting use "phtml" extension for
> > PHP files, and you want to have correct syntax-highlighting for those
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 6cb4280..44094f4 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -3931,15 +3931,16 @@ sub guess_file_syntax {
> > # or return original FD if no highlighting
> > sub run_highlighter {
> > my ($fd, $highlight, $syntax) = @_;
> > - return $fd unless ($highlight && defined $syntax);
> > + return $fd unless ($highlight);
> >
> > close $fd;
> > + my $syntax_arg = (defined $syntax) ? "--syntax $syntax" : "--force";
> > open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
> > quote_command($^X, '-CO', '-MEncode=decode,FB_DEFAULT', '-pse',
> > '$_ = decode($fe, $_, FB_DEFAULT) if !utf8::decode($_);',
> > '--', "-fe=$fallback_encoding")." | ".
> > quote_command($highlight_bin).
> > - " --replace-tabs=8 --fragment --syntax $syntax |"
> > + " --replace-tabs=8 --fragment $syntax_arg |"
> > or die_error(500, "Couldn't open file or run syntax highlighter");
> > return $fd;
> > }
>
> All right, nice and understandable.
>
> > @@ -7063,8 +7064,7 @@ sub git_blob {
> >
> > my $highlight = gitweb_check_feature('highlight');
> > my $syntax = guess_file_syntax($highlight, $file_name);
> > - $fd = run_highlighter($fd, $highlight, $syntax)
> > - if $syntax;
> > + $fd = run_highlighter($fd, $highlight, $syntax);
> >
> > git_header_html(undef, $expires);
> > my $formats_nav = '';
>
> Good, run unconditionally.
>
> > @@ -7117,7 +7117,7 @@ sub git_blob {
> > $line = untabify($line);
> > printf qq!<div class="pre"><a id="l%i" href="%s#l%i" class="linenr">%4i</a> %s</div>\n!,
> > $nr, esc_attr(href(-replay => 1)), $nr, $nr,
> > - $syntax ? sanitize($line) : esc_html($line, -nbsp=>1);
> > + $highlight ? sanitize($line) : esc_html($line, -nbsp=>1);
> > }
>
> Good, use highlighter if possible, not only if syntax is known
> and highlighter is turned on.
>
> Nice and easy to understand after earlier change.
>
> > }
> > close $fd
> > diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
> > index e94b2f1..6d06ed9 100755
> > --- a/t/t9500-gitweb-standalone-no-errors.sh
> > +++ b/t/t9500-gitweb-standalone-no-errors.sh
> > @@ -709,6 +709,14 @@ test_expect_success HIGHLIGHT \
> > git commit -m "Add test.sh" &&
> > gitweb_run "p=.git;a=blob;f=test.sh"'
> >
> > +test_expect_success HIGHLIGHT \
> > + 'syntax highlighting (highlighter language autodetection)' \
> > + 'git config gitweb.highlight yes &&
> > + echo "#!/usr/bin/perl" > test &&
> > + git add test &&
> > + git commit -m "Add test" &&
> > + gitweb_run "p=.git;a=blob;f=test"'
>
> Nice.
>
> > +
> > # ----------------------------------------------------------------------
> > # forks of projects
> >
> >
>
Thanks for great suggestions.
^ permalink raw reply
* [PATCH v4 2/2] gitweb: use highlight's shebang detection
From: Ian Kelling @ 2016-09-24 22:32 UTC (permalink / raw)
To: git; +Cc: jnareb
In-Reply-To: <20160924223258.9449-1-ian@iankelling.org>
The "highlight" binary can, in some cases, determine the language type
by the means of file contents, for example the shebang in the first line
for some scripting languages. Make use of this autodetection for files
which syntax is not known by gitweb. In that case, pass the blob
contents to "highlight --force"; the parameter is needed to make it
always generate HTML output (which includes HTML-escaping).
Although we now run highlight on files which do not end up highlighted,
performance is virtually unaffected because when we call highlight, it
is used for escaping HTML. In the case that highlight is used, gitweb
calls sanitize() instead of esc_html(), and the latter is significantly
slower (it does more, being roughly a superset of sanitize()). Simple
benchmark comparing performance of 'blob' view of files without syntax
highlighting in gitweb before and after this change indicates ±1%
difference in request time for all file types. Benchmark was performed
on local instance on Debian, using Apache/2.4.23 web server and CGI.
Document the feature and improve syntax highlight documentation, add
test to ensure gitweb doesn't crash when language detection is used.
Signed-off-by: Ian Kelling <ian@iankelling.org>
---
Notes:
The only change from v3 is the commit message as suggested by Jakub
Narębski
Documentation/gitweb.conf.txt | 21 ++++++++++++++-------
gitweb/gitweb.perl | 10 +++++-----
t/t9500-gitweb-standalone-no-errors.sh | 8 ++++++++
3 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index a79e350..e632089 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -246,13 +246,20 @@ $highlight_bin::
Note that 'highlight' feature must be set for gitweb to actually
use syntax highlighting.
+
-*NOTE*: if you want to add support for new file type (supported by
-"highlight" but not used by gitweb), you need to modify `%highlight_ext`
-or `%highlight_basename`, depending on whether you detect type of file
-based on extension (for example "sh") or on its basename (for example
-"Makefile"). The keys of these hashes are extension and basename,
-respectively, and value for given key is name of syntax to be passed via
-`--syntax <syntax>` to highlighter.
+*NOTE*: for a file to be highlighted, its syntax type must be detected
+and that syntax must be supported by "highlight". The default syntax
+detection is minimal, and there are many supported syntax types with no
+detection by default. There are three options for adding syntax
+detection. The first and second priority are `%highlight_basename` and
+`%highlight_ext`, which detect based on basename (the full filename, for
+example "Makefile") and extension (for example "sh"). The keys of these
+hashes are the basename and extension, respectively, and the value for a
+given key is the name of the syntax to be passed via `--syntax <syntax>`
+to "highlight". The last priority is the "highlight" configuration of
+`Shebang` regular expressions to detect the language based on the first
+line in the file, (for example, matching the line "#!/bin/bash"). See
+the highlight documentation and the default config at
+/etc/highlight/filetypes.conf for more details.
+
For example if repositories you are hosting use "phtml" extension for
PHP files, and you want to have correct syntax-highlighting for those
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6cb4280..44094f4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3931,15 +3931,16 @@ sub guess_file_syntax {
# or return original FD if no highlighting
sub run_highlighter {
my ($fd, $highlight, $syntax) = @_;
- return $fd unless ($highlight && defined $syntax);
+ return $fd unless ($highlight);
close $fd;
+ my $syntax_arg = (defined $syntax) ? "--syntax $syntax" : "--force";
open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
quote_command($^X, '-CO', '-MEncode=decode,FB_DEFAULT', '-pse',
'$_ = decode($fe, $_, FB_DEFAULT) if !utf8::decode($_);',
'--', "-fe=$fallback_encoding")." | ".
quote_command($highlight_bin).
- " --replace-tabs=8 --fragment --syntax $syntax |"
+ " --replace-tabs=8 --fragment $syntax_arg |"
or die_error(500, "Couldn't open file or run syntax highlighter");
return $fd;
}
@@ -7063,8 +7064,7 @@ sub git_blob {
my $highlight = gitweb_check_feature('highlight');
my $syntax = guess_file_syntax($highlight, $file_name);
- $fd = run_highlighter($fd, $highlight, $syntax)
- if $syntax;
+ $fd = run_highlighter($fd, $highlight, $syntax);
git_header_html(undef, $expires);
my $formats_nav = '';
@@ -7117,7 +7117,7 @@ sub git_blob {
$line = untabify($line);
printf qq!<div class="pre"><a id="l%i" href="%s#l%i" class="linenr">%4i</a> %s</div>\n!,
$nr, esc_attr(href(-replay => 1)), $nr, $nr,
- $syntax ? sanitize($line) : esc_html($line, -nbsp=>1);
+ $highlight ? sanitize($line) : esc_html($line, -nbsp=>1);
}
}
close $fd
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index e94b2f1..6d06ed9 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -709,6 +709,14 @@ test_expect_success HIGHLIGHT \
git commit -m "Add test.sh" &&
gitweb_run "p=.git;a=blob;f=test.sh"'
+test_expect_success HIGHLIGHT \
+ 'syntax highlighting (highlighter language autodetection)' \
+ 'git config gitweb.highlight yes &&
+ echo "#!/usr/bin/perl" > test &&
+ git add test &&
+ git commit -m "Add test" &&
+ gitweb_run "p=.git;a=blob;f=test"'
+
# ----------------------------------------------------------------------
# forks of projects
--
2.9.3
^ permalink raw reply related
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