* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Junio C Hamano @ 2016-10-28 20:38 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Jacob Keller, Johannes Schindelin,
git@vger.kernel.org, Simon Ruderich, Jeff King
In-Reply-To: <CAGZ79kZVivquVDH9S63K8p3bOis+e=b3cKVVrg13zPe_BrL_Sw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Fri, Oct 28, 2016 at 1:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Another problem with the proposed patch is that there is no
>>> declaration for attr_start() before the call in compat/mingw.c. We
>>> would have to move the declaration of attr_start() to cache.h (for
>>> example), because #including attr.h in compat/mingw.c is plainly
>>> wrong. However, it would not be a major offense to #include attr.h in
>>> common-main.c. But when we do that, we can certainly spare the few
>>> cycles to call pthread_mutex_init.
>>
>> That sounds like a good argument to have it in common-main.c.
>
> If we're going that route, I would get rid of PTHREAD_MUTEX_INITILIZER
> and call a pthread_mutex_init platform independently.
Yup, but earlier j6t was trying hard not to penalize any single
platform, and that would certainly penalize the ones with static
initialization, so I was hoping to hear if there is a clever
workaround to avoid that.
>> Would it mean that the code that defines the mutex needs to have
>> #ifdef that defines a no-op attr_start() and defines the mutex with
>> PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex
>> without initializatin, with the real attr_start(), though?
^ permalink raw reply
* Can't get git to stop outputting to StdErr
From: Scott R. Chamberlain @ 2016-10-28 20:31 UTC (permalink / raw)
To: git@vger.kernel.org
I am working on some build scripts that get run on TFS. During the build process I need to check in the changes that where done during the build process (A set of binaries other projects depend on).
I would really like to leave the option "Fail on Standard Error" enabled for the script in TFS, however my push keeps writing to standard error even though I told it not to.
The line I do is:
git push -q binaryRepo HEAD:"$Env:BUILD_SOURCEBRANCH"
But I get the following in my log after the build
2016-10-28T20:05:32.3179442Z ##[error]remote:
remote: Analyzing objects... (3/3) (657 ms)
remote: Storing packfile... done (40 ms)
remote: Storing index... done (42 ms)
2016-10-28T20:05:32.3209423Z Done
2016-10-28T20:05:32.4019436Z ##[error]Process completed with exit code 0 and had 1 error(s) written to the error stream.
2016-10-28T20:05:32.4029436Z ##[debug]System.Exception: Process completed with exit code 0 and had 1 error(s) written to the error stream.
Why am I still getting output to standard error when I included the `-q` switch?
For reference, `git version` reports `2.10.0.windows.1` and HEAD is a detached HEAD.
Scott Chamberlain
Software Engineer
ImproMed, LLC
--
Rely On Us.
ImproMed LLC
Henry Schein Animal Health
--
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Johannes Sixt @ 2016-10-28 20:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jacob Keller, Johannes Schindelin, Stefan Beller,
git@vger.kernel.org, Simon Ruderich, Jeff King
In-Reply-To: <xmqqr370z07v.fsf@gitster.mtv.corp.google.com>
Am 28.10.2016 um 22:29 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Another problem with the proposed patch is that there is no
>> declaration for attr_start() before the call in compat/mingw.c. We
>> would have to move the declaration of attr_start() to cache.h (for
>> example), because #including attr.h in compat/mingw.c is plainly
>> wrong. However, it would not be a major offense to #include attr.h in
>> common-main.c. But when we do that, we can certainly spare the few
>> cycles to call pthread_mutex_init.
>
> That sounds like a good argument to have it in common-main.c.
>
> Would it mean that the code that defines the mutex needs to have
> #ifdef that defines a no-op attr_start() and defines the mutex with
> PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex
> without initializatin, with the real attr_start(), though?
No. My intent was to call pthread_mutex_init for all platforms. We
already call open("/dev/null") unconditionally on every program startup.
The few cycles spent in pthread_mutex_init should be the least of our
worries.
-- Hannes
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Junio C Hamano @ 2016-10-28 20:52 UTC (permalink / raw)
To: Johannes Sixt
Cc: Jacob Keller, Johannes Schindelin, Stefan Beller,
git@vger.kernel.org, Simon Ruderich, Jeff King
In-Reply-To: <ab4c8148-2124-f08d-a770-e4f4e49a1c15@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 28.10.2016 um 22:29 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Another problem with the proposed patch is that there is no
>>> declaration for attr_start() before the call in compat/mingw.c. We
>>> would have to move the declaration of attr_start() to cache.h (for
>>> example), because #including attr.h in compat/mingw.c is plainly
>>> wrong. However, it would not be a major offense to #include attr.h in
>>> common-main.c. But when we do that, we can certainly spare the few
>>> cycles to call pthread_mutex_init.
>>
>> That sounds like a good argument to have it in common-main.c.
>>
>> Would it mean that the code that defines the mutex needs to have
>> #ifdef that defines a no-op attr_start() and defines the mutex with
>> PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex
>> without initializatin, with the real attr_start(), though?
>
> No. My intent was to call pthread_mutex_init for all platforms.
Ah, OK, so there was no magic plan. That's OK.
^ permalink raw reply
* Re: Can't get git to stop outputting to StdErr
From: Junio C Hamano @ 2016-10-28 21:06 UTC (permalink / raw)
To: Scott R. Chamberlain; +Cc: git@vger.kernel.org
In-Reply-To: <c09d32d8ab97418d98ddf356e20a6ff5@ES4.impromed.com>
"Scott R. Chamberlain" <srchamberlain@impromed.com> writes:
> The line I do is:
>
> git push -q binaryRepo HEAD:"$Env:BUILD_SOURCEBRANCH"
This would
(1) squelch the output from the sending side (i.e. local), and
(2) ask "quiet" to the receiving side (i.e. remote), if they know
how to be quiet.
> But I get the following in my log after the build
>
> 2016-10-28T20:05:32.3179442Z ##[error]remote:
> remote: Analyzing objects... (3/3) (657 ms)
> remote: Storing packfile... done (40 ms)
> remote: Storing index... done (42 ms)
These three lines prefixed with "remote:" are coming from the
software that runs on the remote machine that accepts your push, but
the way it says these three things do not look familiar to me. Is
it possible that the remote machine is running a Git server that is
not ours, which lacks the support for "quiet" capability? If that
is the case, the symptom is understandable.
A quick archive search tells me that you are seeing the same issue
as this one:
https://public-inbox.org/git/20160516133731.GA6903@sigill.intra.peff.net/
where the concluding remark, to which I agree, is:
The server side here is clearly not stock git, from the content
of those progress messages (some googling shows it looks like
whatever visualstudio.com is running, but I don't know what that
is). So either the server implementation doesn't support the
"quiet" protocol extension, or it is ignoring it. It might be
worth filing a bug with them.
^ permalink raw reply
* RE: Can't get git to stop outputting to StdErr
From: Scott R. Chamberlain @ 2016-10-28 21:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqq7f8syyhg.fsf@gitster.mtv.corp.google.com>
This is talking to a visualstudio.com git repo. I will file a bug with them.
Scott Chamberlain
Software Engineer
ImproMed, LLC
(800) 925-7171
www.impromed.com
-----Original Message-----
From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Friday, October 28, 2016 4:07 PM
To: Scott R. Chamberlain <srchamberlain@impromed.com>
Cc: git@vger.kernel.org
Subject: Re: Can't get git to stop outputting to StdErr
"Scott R. Chamberlain" <srchamberlain@impromed.com> writes:
> The line I do is:
>
> git push -q binaryRepo HEAD:"$Env:BUILD_SOURCEBRANCH"
This would
(1) squelch the output from the sending side (i.e. local), and
(2) ask "quiet" to the receiving side (i.e. remote), if they know
how to be quiet.
> But I get the following in my log after the build
>
> 2016-10-28T20:05:32.3179442Z ##[error]remote:
> remote: Analyzing objects... (3/3) (657 ms)
> remote: Storing packfile... done (40 ms)
> remote: Storing index... done (42 ms)
These three lines prefixed with "remote:" are coming from the software that runs on the remote machine that accepts your push, but the way it says these three things do not look familiar to me. Is it possible that the remote machine is running a Git server that is not ours, which lacks the support for "quiet" capability? If that is the case, the symptom is understandable.
A quick archive search tells me that you are seeing the same issue as this one:
https://public-inbox.org/git/20160516133731.GA6903@sigill.intra.peff.net/
where the concluding remark, to which I agree, is:
The server side here is clearly not stock git, from the content
of those progress messages (some googling shows it looks like
whatever visualstudio.com is running, but I don't know what that
is). So either the server implementation doesn't support the
"quiet" protocol extension, or it is ignoring it. It might be
worth filing a bug with them.
--
Rely On Us.
ImproMed LLC
Henry Schein Animal Health
--
^ permalink raw reply
* Re: [PATCH] pre-receive.sample: Mark executable
From: Junio C Hamano @ 2016-10-28 21:19 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: git
In-Reply-To: <alpine.DEB.2.10.1610281438000.60842@buzzword-bingo.mit.edu>
Anders Kaseorg <andersk@mit.edu> writes:
> For consistency with other hooks, allow the hook to be activated by
> renaming pre-receive.sample to pre-receive without a separate step to
> mark it executable.
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
Makes sense. Thanks.
> templates/hooks--pre-receive.sample | 0
> 1 file changed, 0 insertions(+), 0 deletions(-)
> mode change 100644 => 100755 templates/hooks--pre-receive.sample
>
> diff --git a/templates/hooks--pre-receive.sample b/templates/hooks--pre-receive.sample
> old mode 100644
> new mode 100755
^ permalink raw reply
* Re: [PATCHv2 28/36] attr: keep attr stack for each check
From: Junio C Hamano @ 2016-10-28 21:35 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, pclouds, git
In-Reply-To: <20161028185502.8789-29-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Instead of having a global attr stack, attach the stack to each check.
> This allows to use the attr in a multithreaded way.
>
>
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> attr.c | 101 +++++++++++++++++++++++++++++++++++++++-----------------------
> attr.h | 4 ++-
> hashmap.h | 2 ++
> 3 files changed, 69 insertions(+), 38 deletions(-)
This looks surprisingly simple ;-) I like it.
I briefly wondered if the addition of lock/unlock surrounding
git_check_attrs() function belongs to [27/36], but that step is not
about making things thread-safe and is primarily to prepare existing
users to use an updated API that can be made thread-safe in later
steps. This [28/36] is the step to have these---so the addition is
not out-of-space at all.
Nicely done.
As this starts to pass a fully populated check object down to the
callchain that begins at bootstrap_attr_stack(), it makes it easier
to add the per-check optimization to read and keep only the relevant
entries from the attribute files later, by passing check also to the
read_attr_from_file() function.
The "set-direction" thing is not yet thread-safe, but I am not sure
what the best way to go there offhand. It somehow feels unnecessary
to allow some thread to be going in the GIT_ATTR_CHECKIN direction
while others to be going in the GIT_ATTR_CHECKOUT direction, so we
probably can leave it at a lower priority for now.
^ permalink raw reply
* Fetch/push lets a malicious server steal the targets of "have" lines
From: Matt McCutchen @ 2016-10-28 21:39 UTC (permalink / raw)
To: git
I was studying the fetch protocol and I realized that in a scenario in
which a client regularly fetches a set of refs from a server and pushes
them back without careful scrutiny, the server can steal the targets of
unrelated refs from the client repository by fabricating its own refs
to the "have" objects specified by the client during the fetch. This
is the reverse of attack #1 described in the "SECURITY" section of the
gitnamespaces(7) man page, with the addition that the server doesn't
have to know the object IDs in advance. Is this supposed to be well-
known? I've been using git since 2006 and it was a surprise to me.
Hopefully it isn't very common for a user to fetch and push with a
server they don't trust to have all the data in their repository. I
don't think I have any such cases myself; I have unfinished work that
isn't meant for scrutiny by others, but nothing really damaging if it
were released to the server. This attack presents no new risks if a
user already runs code fetched from the server in such a way that it
can read the repository. But there might be some users who just review
embargoed security fixes from multiple sources (or something like that)
without running code themselves, and their security expectations might
be violated.
If my analysis is correct, I'd argue for documenting the issue in a
"SECURITY" section in the git-fetch man page. Shall I submit a patch?
Thanks for your attention.
Matt
^ permalink raw reply
* Re: [PATCHv2 00/36] Revamp the attr subsystem!
From: Junio C Hamano @ 2016-10-28 21:43 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, pclouds, git
In-Reply-To: <20161028185502.8789-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> previous discussion at https://public-inbox.org/git/20161022233225.8883-1-sbeller@google.com
>
> This implements the discarded series':
> jc/attr
> jc/attr-more
> sb/pathspec-label
> sb/submodule-default-paths
>
> This includes
> * The fixes for windows
> * Junios latest suggestion to use git_attr_check_initv instead of
> alloc/append.
>
> * I implemented the thread safe attr API in patch 27 (attr: convert to new threadsafe API)
> * patch 28 (attr: keep attr stack for each check) makes it actually possible
> to run in a multithreaded environment.
> * I added a test for the multithreaded when it is introduced in patch 32
> (pathspec: allow querying for attributes) as well as a test to disallow
> multiple "attr"s in a pathspec.
I'd appreciate if you didn't unnecessarily rebase the series. It
would make comparing the new round with the previous one a lot
easier.
Thanks.
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Junio C Hamano @ 2016-10-28 22:00 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1477690790.2904.22.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> I was studying the fetch protocol and I realized that in a scenario in
> which a client regularly fetches a set of refs from a server and pushes
> them back without careful scrutiny, the server can steal the targets of
> unrelated refs from the client repository by fabricating its own refs
> to the "have" objects specified by the client during the fetch.
Let me see if I understood your scenario correctly.
Suppose we start from this history where 'O' are common, your victim
has a 'Y' branch with two commits that are private to it, as well as
a 'X' branch on which it has X1 that it previously obtained from the
server. On the other hand, the server does not know about Y1 or Y2,
and it added one commit X2 to the branch 'x' the victim is
following:
victim server
Y1---Y2
/
---O---O---X1 ---O---O---X1---X2
Then when victim wants to fetch 'x' from the server, it would say
have X1, have Y2, have Y1, have O
and gets told to shut up by the server who heard enough. The
histories on these two parties will then become like this:
victim server
Y1---Y2
/
---O---O---X1---X2 ---O---O---X1---X2
Victim wishes to keep Y1 and Y2 private, but pushes some other
branch (perhaps builds X3 on top of X2 and pushes 'x'). On push
protocol, the server would lie to the victim that it has Y2 without
knowing what they are.
Is that how your attack scenario goes?
^ permalink raw reply
* Re: [PATCHv2 00/36] Revamp the attr subsystem!
From: Stefan Beller @ 2016-10-28 22:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, Duy Nguyen, git@vger.kernel.org
In-Reply-To: <xmqqr370xi8g.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 28, 2016 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> previous discussion at https://public-inbox.org/git/20161022233225.8883-1-sbeller@google.com
>>
>> This implements the discarded series':
>> jc/attr
>> jc/attr-more
>> sb/pathspec-label
>> sb/submodule-default-paths
>>
>> This includes
>> * The fixes for windows
>> * Junios latest suggestion to use git_attr_check_initv instead of
>> alloc/append.
>>
>> * I implemented the thread safe attr API in patch 27 (attr: convert to new threadsafe API)
>> * patch 28 (attr: keep attr stack for each check) makes it actually possible
>> to run in a multithreaded environment.
>> * I added a test for the multithreaded when it is introduced in patch 32
>> (pathspec: allow querying for attributes) as well as a test to disallow
>> multiple "attr"s in a pathspec.
>
> I'd appreciate if you didn't unnecessarily rebase the series. It
> would make comparing the new round with the previous one a lot
> easier.
>
> Thanks.
I can resend on the original base if you want to; I'd need to reroll anyway
now that the agreement is to put the attr_start call not in the Windows
specific parts.
^ permalink raw reply
* Re: [PATCHv2 27/36] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-28 22:06 UTC (permalink / raw)
To: Stefan Beller, Johannes Sixt; +Cc: bmwill, pclouds, git
In-Reply-To: <20161028185502.8789-28-sbeller@google.com>
Probably this needs to be squashed in, now the MinGW discussion has
settled.
attr.c | 2 +-
common-main.c | 2 ++
compat/mingw.c | 4 ----
3 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/attr.c b/attr.c
index 082b5ed343..961218a0d5 100644
--- a/attr.c
+++ b/attr.c
@@ -50,7 +50,7 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
#ifndef NO_PTHREADS
-static pthread_mutex_t attr_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t attr_mutex;
#define attr_lock() pthread_mutex_lock(&attr_mutex)
#define attr_unlock() pthread_mutex_unlock(&attr_mutex)
void attr_start(void) { pthread_mutex_init(&attr_mutex, NULL); }
diff --git a/common-main.c b/common-main.c
index 44a29e8b13..d4699cd404 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,5 +1,6 @@
#include "cache.h"
#include "exec_cmd.h"
+#include "attr.h"
/*
* Many parts of Git have subprograms communicate via pipe, expect the
@@ -32,6 +33,7 @@ int main(int argc, const char **argv)
sanitize_stdfds();
git_setup_gettext();
+ attr_start();
argv[0] = git_extract_argv0_path(argv[0]);
diff --git a/compat/mingw.c b/compat/mingw.c
index 51ed76326b..3fbfda5978 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,7 +5,6 @@
#include "../strbuf.h"
#include "../run-command.h"
#include "../cache.h"
-#include "../attr.h"
#define HCAST(type, handle) ((type)(intptr_t)handle)
@@ -2233,9 +2232,6 @@ void mingw_startup(void)
/* initialize critical section for waitpid pinfo_t list */
InitializeCriticalSection(&pinfo_cs);
- /* initialize critical sections in the attr code */
- attr_start();
-
/* set up default file mode and file modes for stdin/out/err */
_fmode = _O_BINARY;
_setmode(_fileno(stdin), _O_BINARY);
^ permalink raw reply related
* Re: [PATCHv2 27/36] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-28 22:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Brandon Williams, Duy Nguyen, git@vger.kernel.org
In-Reply-To: <xmqqinscxh5g.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 28, 2016 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Probably this needs to be squashed in, now the MinGW discussion has
> settled.
I was about to propose this (and resend it non-rebased).
So I do not resend, but rather ask you to squash this patch?
Thanks,
Stefan
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Matt McCutchen @ 2016-10-28 22:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqmvhoxhfp.fsf@gitster.mtv.corp.google.com>
On Fri, 2016-10-28 at 15:00 -0700, Junio C Hamano wrote:
> Let me see if I understood your scenario correctly.
>
> Suppose we start from this history where 'O' are common, your victim
> has a 'Y' branch with two commits that are private to it, as well as
> a 'X' branch on which it has X1 that it previously obtained from the
> server. On the other hand, the server does not know about Y1 or Y2,
> and it added one commit X2 to the branch 'x' the victim is
> following:
>
> victim server
>
> Y1---Y2
> /
> ---O---O---X1 ---O---O---X1---X2
>
> Then when victim wants to fetch 'x' from the server, it would say
>
> have X1, have Y2, have Y1, have O
>
> and gets told to shut up by the server who heard enough. The
> histories on these two parties will then become like this:
>
>
> victim server
>
> Y1---Y2
> /
> ---O---O---X1---X2 ---O---O---X1---X2
Then the server generates a commit X3 that lists Y2 as a parent, even
though it doesn't have Y2, and advances 'x' to X3. The victim fetches
'x':
victim server
Y1---Y2---- (Y2)
/ \ \
---O---O---X1---X2---X3 ---O---O---X1---X2---X3
Then the server rolls back 'x' to X2:
victim server
Y1---Y2----
/ \
---O---O---X1---X2---X3 ---O---O---X1---X2
And the victim pushes:
victim server
Y1---Y2---- Y1---Y2----
/ \ / \
---O---O---X1---X2---X3 ---O---O---X1---X2---X3
Now the server has the content of Y2.
If the victim is fetching and pulling a whole "directory" of refs, e.g:
fetch: refs/heads/*:refs/remotes/server1/*
push: refs/heads/for-server1/*:refs/heads/*
then instead of generating a merge commit, the server can just generate
another ref 'xx' pointing to Y2, assuming it can entice the victim to
set up a corresponding local branch refs/heads/for-server1/xx and push
it back. Or if the victim is for some reason just mirroring back and
forth:
fetch: refs/heads/*:refs/heads/for-server1/*
push: refs/heads/for-
server1/*:refs/heads/*
then it doesn't have to set up a local branch as separate step.
Matt
^ permalink raw reply
* Re: [PATCHv2 27/36] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-28 22:25 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Brandon Williams, Duy Nguyen, git@vger.kernel.org
In-Reply-To: <CAGZ79kayzZOXpRgwREahKAAj+W6M-c2m6dYf5gL3RCxE5+5zJw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Fri, Oct 28, 2016 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Probably this needs to be squashed in, now the MinGW discussion has
>> settled.
>
> I was about to propose this (and resend it non-rebased).
>
> So I do not resend, but rather ask you to squash this patch?
That's OK. I've queued a "SQUASH???" separately for now and if we
need futher changes, you may want to resend, but I can locally
squash before merging it down to 'next' if it turns out that there
is no more changes necessary.
Thanks.
^ permalink raw reply
* Re: [PATCH] Documenation: fmt-merge-msg: fix markup in example
From: Stefan Christ @ 2016-10-28 23:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqq4m3w33o2.fsf@gitster.mtv.corp.google.com>
Hi,
On Fri, Oct 28, 2016 at 08:15:57AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Fri, Oct 28, 2016 at 12:01:26PM +0200, Stefan Christ wrote:
> >
> >> diff --git a/Documentation/git-fmt-merge-msg.txt b/Documentation/git-fmt-merge-msg.txt
> >> index 6526b17..44892c4 100644
> >> --- a/Documentation/git-fmt-merge-msg.txt
> >> +++ b/Documentation/git-fmt-merge-msg.txt
> >> @@ -60,10 +60,10 @@ merge.summary::
> >> EXAMPLE
> >> -------
> >>
> >> ---
> >> +---------
> >> $ git fetch origin master
> >> $ git fmt-merge-msg --log <$GIT_DIR/FETCH_HEAD
> >> ---
> >> +---------
> >
> > Thanks. Asciidoc generally requires at least 4 delimiter characters to
> > open a delimited block (including a ListingBlock, which is what we want
> > here). There is one exception, "--", which is a generic OpenBlock, which
> > is just used for grouping, and not any special syntactic meaning (so
> > that's why this _didn't_ render the "--", but did render the contents
> > without line breaks).
> >
> > So looks good, modulo the typo in the subject that somebody else pointed
> > out.
>
> Thanks, both. I queued with a bit more enhanced log message while
> fixing the typo.
>
I'm totally ok with these changes. Thanks.
Kind Regards,
Stefan Christ
^ permalink raw reply
* Re: [PATCHv2 00/36] Revamp the attr subsystem!
From: Ramsay Jones @ 2016-10-28 23:59 UTC (permalink / raw)
To: Stefan Beller, gitster; +Cc: bmwill, pclouds, git
In-Reply-To: <20161028185502.8789-1-sbeller@google.com>
On 28/10/16 19:54, Stefan Beller wrote:
> previous discussion at https://public-inbox.org/git/20161022233225.8883-1-sbeller@google.com
>
> This implements the discarded series':
> jc/attr
> jc/attr-more
> sb/pathspec-label
> sb/submodule-default-paths
>
> This includes
> * The fixes for windows
> * Junios latest suggestion to use git_attr_check_initv instead of
> alloc/append.
>
> * I implemented the thread safe attr API in patch 27 (attr: convert to new threadsafe API)
> * patch 28 (attr: keep attr stack for each check) makes it actually possible
> to run in a multithreaded environment.
> * I added a test for the multithreaded when it is introduced in patch 32
> (pathspec: allow querying for attributes) as well as a test to disallow
> multiple "attr"s in a pathspec.
By the end of this series, 'git_attr_counted()' and 'git_attr()' are
both file local symbols and can be marked with static. (I gave up the
search for which actual patch should change the symbols to static).
Also, 'git_attr()' ends up with a single caller, so maybe inline that
call?
I was about to have a moan about PTHREAD_MUTEX_INITIALIZER, since it
causes sparse to issue some warnings, but I see that you have decided
not to use it. So, phew! ;-)
ATB,
Ramsay Jones
^ permalink raw reply
* [PATCH 0/4] Make other git commands use trailer layout
From: Jonathan Tan @ 2016-10-29 0:05 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
This is built off jt/trailer-with-cruft (commit 60ef86a).
This patch set makes "commit -s", "cherry-pick -x", and
"format-patch --signoff" use the new trailer definition implemented in
jt/trailer-with-cruft, with some refactoring along the way. With this
patch set, the aforementioned commands would now handle trailers like
those described in [1].
[1] <84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com>
Jonathan Tan (4):
commit: make ignore_non_trailer take buf/len
trailer: avoid unnecessary splitting on lines
trailer: have function to describe trailer layout
sequencer: use trailer's trailer layout
builtin/commit.c | 2 +-
commit.c | 22 ++--
commit.h | 2 +-
sequencer.c | 75 +++---------
t/t3511-cherry-pick-x.sh | 16 ++-
t/t4014-format-patch.sh | 40 +++++--
t/t7501-commit.sh | 36 ++++++
trailer.c | 295 ++++++++++++++++++++++++++++-------------------
trailer.h | 25 ++++
9 files changed, 313 insertions(+), 200 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH 1/4] commit: make ignore_non_trailer take buf/len
From: Jonathan Tan @ 2016-10-29 0:05 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1477698917.git.jonathantanmy@google.com>
Make ignore_non_trailer take a buf/len pair instead of struct strbuf.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
builtin/commit.c | 2 +-
commit.c | 22 +++++++++++-----------
commit.h | 2 +-
trailer.c | 2 +-
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 1cba3b7..c26b939 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -790,7 +790,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
strbuf_stripspace(&sb, 0);
if (signoff)
- append_signoff(&sb, ignore_non_trailer(&sb), 0);
+ append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
die_errno(_("could not write commit template"));
diff --git a/commit.c b/commit.c
index 856fd4a..f70835a 100644
--- a/commit.c
+++ b/commit.c
@@ -1649,7 +1649,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
}
/*
- * Inspect sb and determine the true "end" of the log message, in
+ * Inspect the given string and determine the true "end" of the log message, in
* order to find where to put a new Signed-off-by: line. Ignored are
* trailing comment lines and blank lines, and also the traditional
* "Conflicts:" block that is not commented out, so that we can use
@@ -1659,37 +1659,37 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
* Returns the number of bytes from the tail to ignore, to be fed as
* the second parameter to append_signoff().
*/
-int ignore_non_trailer(struct strbuf *sb)
+int ignore_non_trailer(const char *buf, size_t len)
{
int boc = 0;
int bol = 0;
int in_old_conflicts_block = 0;
- while (bol < sb->len) {
- char *next_line;
+ while (bol < len) {
+ const char *next_line;
- if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol)))
- next_line = sb->buf + sb->len;
+ if (!(next_line = memchr(buf + bol, '\n', len - bol)))
+ next_line = buf + len;
else
next_line++;
- if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') {
+ if (buf[bol] == comment_line_char || buf[bol] == '\n') {
/* is this the first of the run of comments? */
if (!boc)
boc = bol;
/* otherwise, it is just continuing */
- } else if (starts_with(sb->buf + bol, "Conflicts:\n")) {
+ } else if (starts_with(buf + bol, "Conflicts:\n")) {
in_old_conflicts_block = 1;
if (!boc)
boc = bol;
- } else if (in_old_conflicts_block && sb->buf[bol] == '\t') {
+ } else if (in_old_conflicts_block && buf[bol] == '\t') {
; /* a pathname in the conflicts block */
} else if (boc) {
/* the previous was not trailing comment */
boc = 0;
in_old_conflicts_block = 0;
}
- bol = next_line - sb->buf;
+ bol = next_line - buf;
}
- return boc ? sb->len - boc : 0;
+ return boc ? len - boc : 0;
}
diff --git a/commit.h b/commit.h
index afd14f3..9c12abb 100644
--- a/commit.h
+++ b/commit.h
@@ -355,7 +355,7 @@ extern const char *find_commit_header(const char *msg, const char *key,
size_t *out_len);
/* Find the end of the log message, the right place for a new trailer. */
-extern int ignore_non_trailer(struct strbuf *sb);
+extern int ignore_non_trailer(const char *buf, size_t len);
typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
void *cb_data);
diff --git a/trailer.c b/trailer.c
index d19a92c..6d8375b 100644
--- a/trailer.c
+++ b/trailer.c
@@ -828,7 +828,7 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)
for (i = 0; i < patch_start; i++)
strbuf_addbuf(&sb, lines[i]);
- ignore_bytes = ignore_non_trailer(&sb);
+ ignore_bytes = ignore_non_trailer(sb.buf, sb.len);
strbuf_release(&sb);
for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
ignore_bytes -= lines[i]->len;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 2/4] trailer: avoid unnecessary splitting on lines
From: Jonathan Tan @ 2016-10-29 0:05 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1477698917.git.jonathantanmy@google.com>
trailer.c currently splits lines while processing a buffer (and also
rejoins lines when needing to invoke ignore_non_trailer).
Avoid such line splitting, except when generating the strings
corresponding to trailers (for ease of use by clients - a subsequent
patch will allow other components to obtain the layout of a trailer
block in a buffer, including the trailers themselves). The main purpose
of this is to make it easy to return pointers into the original buffer
(for a subsequent patch), but this also significantly reduces the number
of memory allocations required.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------
1 file changed, 116 insertions(+), 99 deletions(-)
diff --git a/trailer.c b/trailer.c
index 6d8375b..d4d9e10 100644
--- a/trailer.c
+++ b/trailer.c
@@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)
return same_token(a, b) && same_value(a, b);
}
-static inline int contains_only_spaces(const char *str)
+static inline int is_blank_line(const char *str)
{
const char *s = str;
- while (*s && isspace(*s))
+ while (*s && *s != '\n' && isspace(*s))
s++;
- return !*s;
+ return !*s || *s == '\n';
}
static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)
@@ -566,13 +566,18 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
}
/*
- * Return the location of the first separator in line, or -1 if there is no
- * separator.
+ * Return the location of the first separator in the given line, or -1 if there
+ * is no separator.
+ *
+ * The interests parameter must contain the acceptable separators and may
+ * contain '\n'. If interests contains '\n', the input line is considered to
+ * end at the first newline encountered. Otherwise, the input line is
+ * considered to end at its null terminator.
*/
-static int find_separator(const char *line, const char *separators)
+static int find_separator(const char *line, const char *interests)
{
- int loc = strcspn(line, separators);
- if (!line[loc])
+ int loc = strcspn(line, interests);
+ if (!line[loc] || line[loc] == '\n')
return -1;
return loc;
}
@@ -688,51 +693,71 @@ static void process_command_line_args(struct list_head *arg_head,
free(cl_separators);
}
-static struct strbuf **read_input_file(const char *file)
+static void read_input_file(struct strbuf *sb, const char *file)
{
- struct strbuf **lines;
- struct strbuf sb = STRBUF_INIT;
-
if (file) {
- if (strbuf_read_file(&sb, file, 0) < 0)
+ if (strbuf_read_file(sb, file, 0) < 0)
die_errno(_("could not read input file '%s'"), file);
} else {
- if (strbuf_read(&sb, fileno(stdin), 0) < 0)
+ if (strbuf_read(sb, fileno(stdin), 0) < 0)
die_errno(_("could not read from stdin"));
}
+}
- lines = strbuf_split(&sb, '\n');
+static const char *next_line(const char *str)
+{
+ const char *nl = strchrnul(str, '\n');
+ return nl + !!*nl;
+}
- strbuf_release(&sb);
+/*
+ * Return the position of the start of the last line. If len is 0, return -1.
+ */
+static int last_line(const char *buf, size_t len)
+{
+ int i;
+ if (len == 0)
+ return -1;
+ if (len == 1)
+ return 0;
+ /*
+ * Skip the last character (in addition to the null terminator),
+ * because if the last character is a newline, it is considered as part
+ * of the last line anyway.
+ */
+ i = len - 2;
- return lines;
+ for (; i >= 0; i--) {
+ if (buf[i] == '\n')
+ return i + 1;
+ }
+ return 0;
}
/*
- * Return the (0 based) index of the start of the patch or the line
- * count if there is no patch in the message.
+ * Return the position of the start of the patch or the length of str if there
+ * is no patch in the message.
*/
-static int find_patch_start(struct strbuf **lines, int count)
+static int find_patch_start(const char *str)
{
- int i;
+ const char *s;
- /* Get the start of the patch part if any */
- for (i = 0; i < count; i++) {
- if (starts_with(lines[i]->buf, "---"))
- return i;
+ for (s = str; *s; s = next_line(s)) {
+ if (starts_with(s, "---"))
+ return s - str;
}
- return count;
+ return s - str;
}
/*
- * Return the (0 based) index of the first trailer line or count if
- * there are no trailers. Trailers are searched only in the lines from
- * index (count - 1) down to index 0.
+ * Return the position of the first trailer line or len if there are no
+ * trailers.
*/
-static int find_trailer_start(struct strbuf **lines, int count)
+static int find_trailer_start(const char *buf, size_t len)
{
- int start, end_of_title, only_spaces = 1;
+ const char *s;
+ int title_end, l, only_spaces = 1;
int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
/*
* Number of possible continuation lines encountered. This will be
@@ -742,15 +767,18 @@ static int find_trailer_start(struct strbuf **lines, int count)
* are to be considered non-trailers).
*/
int possible_continuation_lines = 0;
+ int ret;
+
+ char *sep_nl = xstrfmt("%s\n", separators);
/* The first paragraph is the title and cannot be trailers */
- for (start = 0; start < count; start++) {
- if (lines[start]->buf[0] == comment_line_char)
+ for (s = buf; s < buf + len; s = next_line(s)) {
+ if (s[0] == comment_line_char)
continue;
- if (contains_only_spaces(lines[start]->buf))
+ if (is_blank_line(s))
break;
}
- end_of_title = start;
+ title_end = s - buf;
/*
* Get the start of the trailers by looking starting from the end for a
@@ -758,30 +786,33 @@ static int find_trailer_start(struct strbuf **lines, int count)
* trailers, or (ii) contains at least one Git-generated trailer and
* consists of at least 25% trailers.
*/
- for (start = count - 1; start >= end_of_title; start--) {
+ for (l = last_line(buf, len); l >= title_end; l = last_line(buf, l)) {
+ const char *bol = buf + l;
const char **p;
int separator_pos;
- if (lines[start]->buf[0] == comment_line_char) {
+ if (bol[0] == comment_line_char) {
non_trailer_lines += possible_continuation_lines;
possible_continuation_lines = 0;
continue;
}
- if (contains_only_spaces(lines[start]->buf)) {
+ if (is_blank_line(bol)) {
if (only_spaces)
continue;
non_trailer_lines += possible_continuation_lines;
if (recognized_prefix &&
trailer_lines * 3 >= non_trailer_lines)
- return start + 1;
- if (trailer_lines && !non_trailer_lines)
- return start + 1;
- return count;
+ ret = next_line(bol) - buf;
+ else if (trailer_lines && !non_trailer_lines)
+ ret = next_line(bol) - buf;
+ else
+ ret = len;
+ goto cleanup;
}
only_spaces = 0;
for (p = git_generated_prefixes; *p; p++) {
- if (starts_with(lines[start]->buf, *p)) {
+ if (starts_with(bol, *p)) {
trailer_lines++;
possible_continuation_lines = 0;
recognized_prefix = 1;
@@ -789,8 +820,8 @@ static int find_trailer_start(struct strbuf **lines, int count)
}
}
- separator_pos = find_separator(lines[start]->buf, separators);
- if (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {
+ separator_pos = find_separator(bol, sep_nl);
+ if (separator_pos >= 1 && !isspace(bol[0])) {
struct list_head *pos;
trailer_lines++;
@@ -800,13 +831,13 @@ static int find_trailer_start(struct strbuf **lines, int count)
list_for_each(pos, &conf_head) {
struct arg_item *item;
item = list_entry(pos, struct arg_item, list);
- if (token_matches_item(lines[start]->buf, item,
+ if (token_matches_item(bol, item,
separator_pos)) {
recognized_prefix = 1;
break;
}
}
- } else if (isspace(lines[start]->buf[0]))
+ } else if (isspace(bol[0]))
possible_continuation_lines++;
else {
non_trailer_lines++;
@@ -817,88 +848,73 @@ static int find_trailer_start(struct strbuf **lines, int count)
;
}
- return count;
+ ret = len;
+cleanup:
+ free(sep_nl);
+ return ret;
}
-/* Get the index of the end of the trailers */
-static int find_trailer_end(struct strbuf **lines, int patch_start)
+/* Return the position of the end of the trailers. */
+static int find_trailer_end(const char *buf, size_t len)
{
- struct strbuf sb = STRBUF_INIT;
- int i, ignore_bytes;
-
- for (i = 0; i < patch_start; i++)
- strbuf_addbuf(&sb, lines[i]);
- ignore_bytes = ignore_non_trailer(sb.buf, sb.len);
- strbuf_release(&sb);
- for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
- ignore_bytes -= lines[i]->len;
-
- return i + 1;
+ return len - ignore_non_trailer(buf, len);
}
-static int has_blank_line_before(struct strbuf **lines, int start)
+static int ends_with_blank_line(const char *buf, size_t len)
{
- for (;start >= 0; start--) {
- if (lines[start]->buf[0] == comment_line_char)
- continue;
- return contains_only_spaces(lines[start]->buf);
- }
- return 0;
-}
-
-static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)
-{
- int i;
- for (i = start; lines[i] && i < end; i++)
- fprintf(outfile, "%s", lines[i]->buf);
+ int ll = last_line(buf, len);
+ if (ll < 0)
+ return 0;
+ return is_blank_line(buf + ll);
}
static int process_input_file(FILE *outfile,
- struct strbuf **lines,
+ const char *str,
struct list_head *head)
{
- int count = 0;
- int patch_start, trailer_start, trailer_end, i;
+ int patch_start, trailer_start, trailer_end;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
struct trailer_item *last = NULL;
+ struct strbuf *trailer, **trailer_lines, **ptr;
- /* Get the line count */
- while (lines[count])
- count++;
-
- patch_start = find_patch_start(lines, count);
- trailer_end = find_trailer_end(lines, patch_start);
- trailer_start = find_trailer_start(lines, trailer_end);
+ patch_start = find_patch_start(str);
+ trailer_end = find_trailer_end(str, patch_start);
+ trailer_start = find_trailer_start(str, trailer_end);
/* Print lines before the trailers as is */
- print_lines(outfile, lines, 0, trailer_start);
+ fwrite(str, 1, trailer_start, outfile);
- if (!has_blank_line_before(lines, trailer_start - 1))
+ if (!ends_with_blank_line(str, trailer_start))
fprintf(outfile, "\n");
/* Parse trailer lines */
- for (i = trailer_start; i < trailer_end; i++) {
+ trailer_lines = strbuf_split_buf(str + trailer_start,
+ trailer_end - trailer_start,
+ '\n',
+ 0);
+ for (ptr = trailer_lines; *ptr; ptr++) {
int separator_pos;
- if (lines[i]->buf[0] == comment_line_char)
+ trailer = *ptr;
+ if (trailer->buf[0] == comment_line_char)
continue;
- if (last && isspace(lines[i]->buf[0])) {
+ if (last && isspace(trailer->buf[0])) {
struct strbuf sb = STRBUF_INIT;
- strbuf_addf(&sb, "%s\n%s", last->value, lines[i]->buf);
+ strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
strbuf_strip_suffix(&sb, "\n");
free(last->value);
last->value = strbuf_detach(&sb, NULL);
continue;
}
- separator_pos = find_separator(lines[i]->buf, separators);
+ separator_pos = find_separator(trailer->buf, separators);
if (separator_pos >= 1) {
- parse_trailer(&tok, &val, NULL, lines[i]->buf,
+ parse_trailer(&tok, &val, NULL, trailer->buf,
separator_pos);
last = add_trailer_item(head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL));
} else {
- strbuf_addbuf(&val, lines[i]);
+ strbuf_addbuf(&val, trailer);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(head,
NULL,
@@ -906,6 +922,7 @@ static int process_input_file(FILE *outfile,
last = NULL;
}
}
+ strbuf_list_free(trailer_lines);
return trailer_end;
}
@@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
{
LIST_HEAD(head);
LIST_HEAD(arg_head);
- struct strbuf **lines;
+ struct strbuf sb = STRBUF_INIT;
int trailer_end;
FILE *outfile = stdout;
@@ -962,13 +979,13 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
git_config(git_trailer_default_config, NULL);
git_config(git_trailer_config, NULL);
- lines = read_input_file(file);
+ read_input_file(&sb, file);
if (in_place)
outfile = create_in_place_tempfile(file);
/* Print the lines before the trailers */
- trailer_end = process_input_file(outfile, lines, &head);
+ trailer_end = process_input_file(outfile, sb.buf, &head);
process_command_line_args(&arg_head, trailers);
@@ -979,11 +996,11 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
free_all(&head);
/* Print the lines after the trailers as is */
- print_lines(outfile, lines, trailer_end, INT_MAX);
+ fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
if (in_place)
if (rename_tempfile(&trailers_tempfile, file))
die_errno(_("could not rename temporary file to %s"), file);
- strbuf_list_free(lines);
+ strbuf_release(&sb);
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 3/4] trailer: have function to describe trailer layout
From: Jonathan Tan @ 2016-10-29 0:05 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1477698917.git.jonathantanmy@google.com>
Create a function that, taking a string, describes the position of its
trailer block (if available) and the contents thereof, and make trailer
use it. This makes it easier for other Git components, in the future, to
interpret trailer blocks in the same way as trailer.
In a subsequent patch, another component will be made to use this.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
trailer.c | 120 +++++++++++++++++++++++++++++++++++++++++++-------------------
trailer.h | 25 +++++++++++++
2 files changed, 108 insertions(+), 37 deletions(-)
diff --git a/trailer.c b/trailer.c
index d4d9e10..2f5c815 100644
--- a/trailer.c
+++ b/trailer.c
@@ -46,6 +46,8 @@ static LIST_HEAD(conf_head);
static char *separators = ":";
+static int configured = 0;
+
#define TRAILER_ARG_STRING "$ARG"
static const char *git_generated_prefixes[] = {
@@ -549,6 +551,17 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
return 0;
}
+static void ensure_configured(void)
+{
+ if (configured)
+ return;
+
+ /* Default config must be setup first */
+ git_config(git_trailer_default_config, NULL);
+ git_config(git_trailer_config, NULL);
+ configured = 1;
+}
+
static const char *token_from_item(struct arg_item *item, char *tok)
{
if (item->conf.key)
@@ -872,59 +885,43 @@ static int process_input_file(FILE *outfile,
const char *str,
struct list_head *head)
{
- int patch_start, trailer_start, trailer_end;
+ struct trailer_info info;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
- struct trailer_item *last = NULL;
- struct strbuf *trailer, **trailer_lines, **ptr;
+ int i;
- patch_start = find_patch_start(str);
- trailer_end = find_trailer_end(str, patch_start);
- trailer_start = find_trailer_start(str, trailer_end);
+ trailer_info_get(&info, str);
/* Print lines before the trailers as is */
- fwrite(str, 1, trailer_start, outfile);
+ fwrite(str, 1, info.trailer_start - str, outfile);
- if (!ends_with_blank_line(str, trailer_start))
+ if (!info.blank_line_before_trailer)
fprintf(outfile, "\n");
- /* Parse trailer lines */
- trailer_lines = strbuf_split_buf(str + trailer_start,
- trailer_end - trailer_start,
- '\n',
- 0);
- for (ptr = trailer_lines; *ptr; ptr++) {
+ for (i = 0; i < info.trailer_nr; i++) {
int separator_pos;
- trailer = *ptr;
- if (trailer->buf[0] == comment_line_char)
+ char *trailer = info.trailers[i];
+ if (trailer[0] == comment_line_char)
continue;
- if (last && isspace(trailer->buf[0])) {
- struct strbuf sb = STRBUF_INIT;
- strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
- strbuf_strip_suffix(&sb, "\n");
- free(last->value);
- last->value = strbuf_detach(&sb, NULL);
- continue;
- }
- separator_pos = find_separator(trailer->buf, separators);
+ separator_pos = find_separator(trailer, separators);
if (separator_pos >= 1) {
- parse_trailer(&tok, &val, NULL, trailer->buf,
- separator_pos);
- last = add_trailer_item(head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ parse_trailer(&tok, &val, NULL, trailer,
+ separator_pos);
+ add_trailer_item(head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL));
} else {
- strbuf_addbuf(&val, trailer);
+ strbuf_addstr(&val, trailer);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(head,
NULL,
strbuf_detach(&val, NULL));
- last = NULL;
}
}
- strbuf_list_free(trailer_lines);
- return trailer_end;
+ trailer_info_release(&info);
+
+ return info.trailer_end - str;
}
static void free_all(struct list_head *head)
@@ -975,9 +972,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
int trailer_end;
FILE *outfile = stdout;
- /* Default config must be setup first */
- git_config(git_trailer_default_config, NULL);
- git_config(git_trailer_config, NULL);
+ ensure_configured();
read_input_file(&sb, file);
@@ -1004,3 +999,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
strbuf_release(&sb);
}
+
+void trailer_info_get(struct trailer_info *info, const char *str)
+{
+ int patch_start, trailer_end, trailer_start;
+ struct strbuf **trailer_lines, **ptr;
+ char **trailer_strings = NULL;
+ size_t nr = 0, alloc = 0;
+ char **last = NULL;
+
+ ensure_configured();
+
+ patch_start = find_patch_start(str);
+ trailer_end = find_trailer_end(str, patch_start);
+ trailer_start = find_trailer_start(str, trailer_end);
+
+ trailer_lines = strbuf_split_buf(str + trailer_start,
+ trailer_end - trailer_start,
+ '\n',
+ 0);
+ for (ptr = trailer_lines; *ptr; ptr++) {
+ if (last && isspace((*ptr)->buf[0])) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
+ strbuf_addbuf(&sb, *ptr);
+ *last = strbuf_detach(&sb, NULL);
+ continue;
+ }
+ ALLOC_GROW(trailer_strings, nr + 1, alloc);
+ trailer_strings[nr] = strbuf_detach(*ptr, NULL);
+ last = find_separator(trailer_strings[nr], separators) >= 1
+ ? &trailer_strings[nr]
+ : NULL;
+ nr++;
+ }
+ strbuf_list_free(trailer_lines);
+
+ info->blank_line_before_trailer = ends_with_blank_line(str,
+ trailer_start);
+ info->trailer_start = str + trailer_start;
+ info->trailer_end = str + trailer_end;
+ info->trailers = trailer_strings;
+ info->trailer_nr = nr;
+}
+
+void trailer_info_release(struct trailer_info *info)
+{
+ int i;
+ for (i = 0; i < info->trailer_nr; i++)
+ free(info->trailers[i]);
+ free(info->trailers);
+}
diff --git a/trailer.h b/trailer.h
index 36b40b8..65cc5d7 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,7 +1,32 @@
#ifndef TRAILER_H
#define TRAILER_H
+struct trailer_info {
+ /*
+ * True if there is a blank line before the location pointed to by
+ * trailer_start.
+ */
+ int blank_line_before_trailer;
+
+ /*
+ * Pointers to the start and end of the trailer block found. If there
+ * is no trailer block found, these 2 pointers point to the end of the
+ * input string.
+ */
+ const char *trailer_start, *trailer_end;
+
+ /*
+ * Array of trailers found.
+ */
+ char **trailers;
+ size_t trailer_nr;
+};
+
void process_trailers(const char *file, int in_place, int trim_empty,
struct string_list *trailers);
+void trailer_info_get(struct trailer_info *info, const char *str);
+
+void trailer_info_release(struct trailer_info *info);
+
#endif /* TRAILER_H */
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 4/4] sequencer: use trailer's trailer layout
From: Jonathan Tan @ 2016-10-29 0:05 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1477698917.git.jonathantanmy@google.com>
Make sequencer use trailer.c's trailer layout definition, as opposed to
parsing the footer by itself. This makes "commit -s", "cherry-pick -x",
and "format-patch --signoff" consistent with trailer, allowing
non-trailer lines and multiple-line trailers in trailer blocks under
certain conditions, and therefore suppressing the extra newline in those
cases.
Consistency with trailer extends to respecting trailer configs. Tests
have been included to show that.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
sequencer.c | 75 +++++++++---------------------------------------
t/t3511-cherry-pick-x.sh | 16 +++++++++--
t/t4014-format-patch.sh | 40 +++++++++++++++++++++-----
t/t7501-commit.sh | 36 +++++++++++++++++++++++
4 files changed, 96 insertions(+), 71 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index eec8a60..ec0157d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -15,6 +15,7 @@
#include "merge-recursive.h"
#include "refs.h"
#include "argv-array.h"
+#include "trailer.h"
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -26,30 +27,6 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
-static int is_rfc2822_line(const char *buf, int len)
-{
- int i;
-
- for (i = 0; i < len; i++) {
- int ch = buf[i];
- if (ch == ':')
- return 1;
- if (!isalnum(ch) && ch != '-')
- break;
- }
-
- return 0;
-}
-
-static int is_cherry_picked_from_line(const char *buf, int len)
-{
- /*
- * We only care that it looks roughly like (cherry picked from ...)
- */
- return len > strlen(cherry_picked_prefix) + 1 &&
- starts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';
-}
-
/*
* Returns 0 for non-conforming footer
* Returns 1 for conforming footer
@@ -59,49 +36,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)
static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
int ignore_footer)
{
- char prev;
- int i, k;
- int len = sb->len - ignore_footer;
- const char *buf = sb->buf;
- int found_sob = 0;
-
- /* footer must end with newline */
- if (!len || buf[len - 1] != '\n')
- return 0;
+ struct trailer_info info;
+ int i;
+ int found_sob = 0, found_sob_last = 0;
- prev = '\0';
- for (i = len - 1; i > 0; i--) {
- char ch = buf[i];
- if (prev == '\n' && ch == '\n') /* paragraph break */
- break;
- prev = ch;
- }
+ trailer_info_get(&info, sb->buf);
- /* require at least one blank line */
- if (prev != '\n' || buf[i] != '\n')
+ if (info.trailer_start == info.trailer_end)
return 0;
- /* advance to start of last paragraph */
- while (i < len - 1 && buf[i] == '\n')
- i++;
-
- for (; i < len; i = k) {
- int found_rfc2822;
-
- for (k = i; k < len && buf[k] != '\n'; k++)
- ; /* do nothing */
- k++;
+ for (i = 0; i < info.trailer_nr; i++)
+ if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
+ found_sob = 1;
+ if (i == info.trailer_nr - 1)
+ found_sob_last = 1;
+ }
- found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
- if (found_rfc2822 && sob &&
- !strncmp(buf + i, sob->buf, sob->len))
- found_sob = k;
+ trailer_info_release(&info);
- if (!(found_rfc2822 ||
- is_cherry_picked_from_line(buf + i, k - i - 1)))
- return 0;
- }
- if (found_sob == i)
+ if (found_sob_last)
return 3;
if (found_sob)
return 2;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..bf0a5c9 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor <buthor@example.com>"
mesg_broken_footer="$mesg_no_footer
-The signed-off-by string should begin with the words Signed-off-by followed
-by a colon and space, and then the signers name and email address. e.g.
-Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+This is not recognized as a footer because Myfooter is not a recognized token.
+Myfooter: A.U. Thor <author@example.com>"
mesg_with_footer_sob="$mesg_with_footer
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
@@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot
test_cmp expect actual
'
+test_expect_success 'cherry-pick -s recognizes trailer config' '
+ pristine_detach initial &&
+ git -c "trailer.Myfooter.ifexists=add" cherry-pick -s mesg-broken-footer &&
+ cat <<-EOF >expect &&
+ $mesg_broken_footer
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '
pristine_detach initial &&
sha1=$(git rev-parse mesg-no-footer^0) &&
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba4902d..635b394 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1277,8 +1277,7 @@ EOF
4:Subject: [PATCH] subject
8:
9:I want to mention about Signed-off-by: here.
-10:
-11:Signed-off-by: C O Mitter <committer@example.com>
+10:Signed-off-by: C O Mitter <committer@example.com>
EOF
test_cmp expected actual
'
@@ -1294,8 +1293,7 @@ EOF
4:Subject: [PATCH] subject
8:
10:Signed-off-by: example happens to be wrapped here.
-11:
-12:Signed-off-by: C O Mitter <committer@example.com>
+11:Signed-off-by: C O Mitter <committer@example.com>
EOF
test_cmp expected actual
'
@@ -1368,7 +1366,7 @@ EOF
test_cmp expected actual
'
-test_expect_success 'signoff: detect garbage in non-conforming footer' '
+test_expect_success 'signoff: tolerate garbage in conforming footer' '
append_signoff <<\EOF >actual &&
subject
@@ -1383,8 +1381,36 @@ EOF
8:
10:
13:Signed-off-by: C O Mitter <committer@example.com>
-14:
-15:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: respect trailer config' '
+ append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual &&
+
+ test_config trailer.Myfooter.ifexists add &&
+ append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:Signed-off-by: C O Mitter <committer@example.com>
EOF
test_cmp expected actual
'
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a..4003a27 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -460,6 +460,42 @@ $alt" &&
test_cmp expected actual
'
+test_expect_success 'signoff respects trailer config' '
+
+ echo 5 >positive &&
+ git add positive &&
+ git commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+ git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+ (
+ echo subject
+ echo
+ echo non-trailer line
+ echo Myfooter: x
+ echo
+ echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+ ) >expected &&
+ test_cmp expected actual &&
+
+ echo 6 >positive &&
+ git add positive &&
+ git -c "trailer.Myfooter.ifexists=add" commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+ git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+ (
+ echo subject
+ echo
+ echo non-trailer line
+ echo Myfooter: x
+ echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+ ) >expected &&
+ test_cmp expected actual
+'
+
test_expect_success 'multiple -m' '
>negative &&
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Junio C Hamano @ 2016-10-29 1:11 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1477692961.2904.36.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> Then the server generates a commit X3 that lists Y2 as a parent, even
> though it doesn't have Y2, and advances 'x' to X3. The victim fetches
> 'x':
>
> victim server
>
> Y1---Y2---- (Y2)
> / \ \
> ---O---O---X1---X2---X3 ---O---O---X1---X2---X3
>
> Then the server rolls back 'x' to X2:
>
> victim server
>
> Y1---Y2----
> / \
> ---O---O---X1---X2---X3 ---O---O---X1---X2
Ah, I see. My immediate reaction is that you can do worse things in
the reverse direction compared to this, but your scenario does sound
bad already.
^ permalink raw reply
* Re: [PATCH 0/4] Make other git commands use trailer layout
From: Junio C Hamano @ 2016-10-29 1:12 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <cover.1477698917.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> This is built off jt/trailer-with-cruft (commit 60ef86a).
>
> This patch set makes "commit -s", "cherry-pick -x", and
> "format-patch --signoff" use the new trailer definition implemented in
> jt/trailer-with-cruft, with some refactoring along the way. With this
> patch set, the aforementioned commands would now handle trailers like
> those described in [1].
>
> [1] <84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com>
Ooooh. Looks delicious ;-)
^ 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