* Re: [PATCH v3 25/25] sequencer: mark all error messages for translation
From: Johannes Schindelin @ 2016-10-13 14:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Jakub Narębski
In-Reply-To: <xmqqlgxt2rch.fsf@gitster.mtv.corp.google.com>
Hi,
On Wed, 12 Oct 2016, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > Can we please have the following change instead? I think it makes sense
> > to deviate from the usual conventions in a case like this.
>
> You have at least two independent changes relative to Dscho's
> version.
>
> (1) Show line breaks more prominently by avoiding "\n\n" and
> breaking the string at "\n"; this matches the way how the
> result would be displayed more closely to how the source looks
> like.
>
> (2) Ignore the usual indentation rule and have messages start at
> the left end of the source.
>
> Which one are you saying "makes sense" to? Both?
>
> I guess both can be grouped together into one theme: match the way
> the final output and the source code look like.
>
> If that is the motivation behind "makes sense", I'd prefer to see
> the change explained explicitly with that rationale in the log
> message.
>
> Thanks. I personally agree with that motivation (if the one I
> guessed above is your motivation, that is).
I agree with that motivation, but I decided to go about it in a way that
is more in line with the existing source code:
-- snipsnap --
diff --git a/sequencer.c b/sequencer.c
index 8e10bb5..1cf70f7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -483,6 +483,20 @@ static char **read_author_script(void)
return env;
}
+static const char staged_changes_advice[] =
+N_("you have staged changes in your working tree\n"
+"If these changes are meant to be squashed into the previous commit, run:\n"
+"\n"
+" git commit --amend %s\n"
+"\n"
+"If they are meant to go into a new commit, run:\n"
+"\n"
+" git commit %s\n"
+"\n"
+"In both cases, once you're done, continue with:\n"
+"\n"
+" git rebase --continue\n");
+
/*
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
@@ -509,18 +523,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
if (!env) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
- return error(_("you have staged changes in your "
- "working tree. If these changes are "
- "meant to be\n"
- "squashed into the previous commit, "
- "run:\n\n"
- " git commit --amend %s\n\n"
- "If they are meant to go into a new "
- "commit, run:\n\n"
- " git commit %s\n\n"
- "In both cases, once you're done, "
- "continue with:\n\n"
- " git rebase --continue\n"),
+ return error(_(staged_changes_advice),
gpg_opt, gpg_opt);
}
}
^ permalink raw reply related
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-13 14:50 UTC (permalink / raw)
To: Vegard Nossum
Cc: Junio C Hamano, git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1d5dd36b-7a9e-ac00-352a-d71e0e277002@oracle.com>
On Thu, Oct 13, 2016 at 09:17:34AM +0200, Vegard Nossum wrote:
> Oops. I disabled gc a while ago; one reason I did that is that it takes
> a long time to run and it has a tendency to kick in at the worst time. I
> guess I should really put it in cron then.
>
> I'm not sure if this is related, but I also had a problem with GitPython
> and large pack files in the past (" ValueError: Couldn't obtain fanout
> table or warning: packfile ./objects/pack/....pack cannot be accessed")
Sounds like they didn't correctly implement the extra index fanout that
happens for pack above 2G. The old Grit library had a similar bug.
> and I have pack.packSizeLimit set to 512m to fix that.
> Although the whole repo is 17G so I guess it shouldn't be necessary to
> have that many pack files.
Using packSizeLimit does "solve" that problem, but it comes with its own
set of issues. There is a very good chance that your repository would be
much smaller than 17G as a single packfile, because Git does not allow
deltas across packs, and it does not optimize the placement of objects
to keep delta-related objects in a single pack. So you'll quite often be
storing full copies of objects that could otherwise be stored as a tiny
delta.
You might want to compare the resulting size for a full repack with and
without pack.packSizeLimit.
But I agree that is not the cause of your thousand packs. They are more
likely the accumulated cruft of a thousand fetches.
-Peff
^ permalink raw reply
* RE: [External] Re: Fork Errors
From: Vacha, Brian [USA] @ 2016-10-13 13:45 UTC (permalink / raw)
To: Konstantin Khomoutov; +Cc: git@vger.kernel.org
In-Reply-To: <20161006174041.4225c2b52bd144e5b3db8a85@domain007.com>
Thanks, Khomoutov. I turned out that when I went back to version 2.7.0 of Git (as mentioned in this post https://github.com/git-for-windows/git/issues/776) that I no longer received the fork errors. However, then I received a Permission error and also wasn't thrilled that I had to use an old Git. After some installing and uninstalling different ways, I remembered that when things were working for me in the past, I had the GitHub shell installed. So, I installed the GitHub shell with the newest version of Git and was able to use its Git Shell without problem. Now, I can build my app with npm successfully.
-----Original Message-----
From: Konstantin Khomoutov [mailto:kostix+git@007spb.ru]
Sent: Thursday, October 06, 2016 10:41 AM
To: Vacha, Brian [USA] <vacha_brian@bah.com>
Cc: git@vger.kernel.org
Subject: [External] Re: Fork Errors
On Thu, 6 Oct 2016 14:02:09 +0000
"Vacha, Brian [USA]" <vacha_brian@bah.com> wrote:
> When starting Git Bash, I receive the following errors:
> 0 [main] bash 18088 fork: child 14072 - died waiting for dll loading,
> errno 11 bash: fork: retry: No child processes
> 1190419 [main] bash 18088 fork: child 8744 - died waiting for dll
> loading, errno 11 bash: fork: retry: No child processes
> 3343518 [main] bash 18088 fork: child 12324 - died waiting for dll
> loading, errno 11 bash: fork: retry: No child processes
> 7480858 [main] bash 18088 fork: child 17008 - died waiting for dll
> loading, errno 11 bash: fork: retry: No child processes
> 15635036 [main] bash 18088 fork: child 8108 - died waiting for dll
> loading, errno 11 bash: fork: Resource temporarily unavailable
> bash-4.3$
>
> My connection is great at 72 Mbps download and 93 Mbps upload. I
> don't receive other errors so it appears to be a Git Bash issue to me.
Have you tried searching through Git for Windows bugtracker [1] for your problem. I'm pretty sure it was recently discussed there.
The issue #776 [2] looks like the one you're experiencing.
1. https://github.com/git-for-windows/git/issues
2. https://github.com/git-for-windows/git/issues/776
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Johannes Schindelin @ 2016-10-13 12:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqqk2de7e70.fsf@gitster.mtv.corp.google.com>
Hi,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> > On Tue, Oct 11, 2016 at 2:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> * sb/submodule-ignore-trailing-slash (2016-10-10) 2 commits
> >> (merged to 'next' on 2016-10-11 at e37425ed17)
> >> + submodule: ignore trailing slash in relative url
> >> + submodule: ignore trailing slash on superproject URL
> >>
> >> A minor regression fix for "git submodule".
> >>
> >> Will merge to 'master'.
> >
> > Going by the bug report, this *may* be more than
> > minor and worth merging down to maint as well, eventually.
>
> The topic was forked at a reasonably old commit so that it can be
> merged as far down to maint-2.9 if we wanted to. Which means the
> regression was fairly old and fix is not all that urgent as well.
And if you merge it to `master` and `maint`, I will humbly request to do
that at the same time as whatever fix for the regression I reported we
settle on.
I would *hate* to have a `master` (let alone a `maint`) that breaks in Git
for Windows SDK.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
From: Johannes Schindelin @ 2016-10-13 11:11 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Karl A., Dennis Kaarsemaker
In-Reply-To: <CAGZ79kYDpth7YDbN0VRD0dcpp7aeQ-y4HSEhsmd_c46ggZoXsg@mail.gmail.com>
Hi Stefan,
On Wed, 12 Oct 2016, Stefan Beller wrote:
> On Wed, Oct 12, 2016 at 6:30 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Mon, 10 Oct 2016, Stefan Beller wrote:
> >
> >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> >> index 444ec06..a7841a5 100644
> >> --- a/builtin/submodule--helper.c
> >> +++ b/builtin/submodule--helper.c
> >> @@ -95,6 +95,8 @@ static int chop_last_dir(char **remoteurl, int is_relative)
> >> * NEEDSWORK: This works incorrectly on the domain and protocol part.
> >> * remote_url url outcome expectation
> >> * http://a.com/b ../c http://a.com/c as is
> >> + * http://a.com/b/ ../c http://a.com/c same as previous line, but
> >> + * ignore trailing slash in url
> >> * http://a.com/b ../../c http://c error out
> >> * http://a.com/b ../../../c http:/c error out
> >> * http://a.com/b ../../../../c http:c error out
> >> @@ -113,8 +115,8 @@ static char *relative_url(const char *remote_url,
> >> struct strbuf sb = STRBUF_INIT;
> >> size_t len = strlen(remoteurl);
> >>
> >> - if (is_dir_sep(remoteurl[len]))
> >> - remoteurl[len] = '\0';
> >> + if (is_dir_sep(remoteurl[len-1]))
> >> + remoteurl[len-1] = '\0';
> >>
> >> if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
> >> is_relative = 0;
> >> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> >> index bf2deee..82b98f8 100755
> >> --- a/t/t0060-path-utils.sh
> >> +++ b/t/t0060-path-utils.sh
> >> @@ -319,6 +319,7 @@ test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
> >> test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
> >>
> >> test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
> >> +test_submodule_relative_url "(null)" "../foo/bar/" "../sub/a/b/c" "../foo/sub/a/b/c"
> >> test_submodule_relative_url "(null)" "../foo/bar" "../submodule" "../foo/submodule"
> >> test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" "../foo/submodule"
> >> test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
> >
> > I see that this already made it to `next`. I saw that because it breaks
> > the build of Git for Windows (this was not noticed earlier because other
> > compile failures prevented the tests from running), as now the test cases
> > 173 and 177 of t0060 fail (*not* the newly introduced 163).
> >
> > Here is the output with -v -x:
> >
> > -- snip --
> > [...]
> > expecting success:
> > actual=$(git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
> > test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
> >
> > +++ git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../.
> > ++ actual=C:/git-sdk-64/usr/src/git/wip/t/.
> > ++ test C:/git-sdk-64/usr/src/git/wip/t/. = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
>
> So this wipes away one dir too much in a test that doesn't end with a
> dir separator
The problem is not *that* simple. You see, on Windows, there are no Unixy
paths (I used to say POSIX but that is not correct, if you think of VMS
paths looking quite a bit different from what Git expects). To appease
Git's assumption about the exact form of paths, the Bash (actually, the
POSIX emulation layer called MSYS2) converts paths of the form
/c/Windows/system32/drivers/etc/hosts to
C:/Windows/system32/drivers/etc/hosts.
Please note that paths that are already in the latter form are not
touched.
And note also that URLs (actually, anything matching "^[A-Za-z]+://") are
*also* not converted.
The paths that *are* converted can also be of the form /etc/passwd, in
which case the path is prefixed with the Windows directory in which whose
usr/bin/ subdirectory the MSYS2 runtime lives.
In that latter case, i.e. Unixy paths being converted to Windows ones, the
very special case of a trailing "/." is truncated to "/" (IIRC there are
some Windows programs that do not take well to "." referring to a
directory, but my memory on that is flakey).
> (In Windows that is '/' and '\' only, no dots?)
Most Windows functions handle forward slashes just fine. Certainly all
functions involved in the code path in question.
> > One very, very ugly workaround for this newly-introduced breakage would be
> > this:
> >
> > -- snip --
> > diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> > index 82b98f8..abd82e9 100755
> > --- a/t/t0060-path-utils.sh
> > +++ b/t/t0060-path-utils.sh
> > @@ -328,11 +328,11 @@ test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
> > test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
> > test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
> > test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
> > -test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
> > +test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
> > test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
> > test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
> > test_submodule_relative_url "(null)" "$PWD" "./ " "$(pwd)/ "
> > -test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
> > +test_submodule_relative_url "(null)" "$(pwd)/." "../submodule" "$(pwd)/submodule"
> > test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
> > test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
> > test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
> > -- snap --
> >
> > The reasons this is ugly: we specifically test for *Unixy* paths when we
> > use $PWD, as opposed to *Windowsy* paths when using $(pwd). We do this to
> > ensure a certain level of confidence that running things such as
> >
> > git clone --recurse-submodules /z/project/.
> >
> > work. And now that does not work anymore.
>
> After a while of thinking how I could fix it, it occurs to me, I could
> claim the removal of the dot as a defect in the Windows path handling. ;)
Not *quite*. It is not Windows' path handling. It is MSYS2's path
handling, and they must have had good reasons to introduce it. They do not
strip trailing dots just for fun.
> But that doesn't help users.
Exactly.
> Would it be possible to mark the last dir separator special once the
> trailing dot is removed? (i.e. put a \ there, and in this patch we
> only check for /)
> Sounds hacky to me, though.
We could claim that cloning recursively from absolute, Unixy paths is not
supported on Windows.
Given that it still works with relative paths and with absolute Windows
paths and with URLs, I would claim that this is a fair trade-off.
In which case the ugly patch quoted above may be the best way forward.
> > So where to go from here?
>
> So IIUC this patch fixed a bug in Git and introduced a very similar bug
> in Git for Windows?
Yep. Even if it fixed the very same bug on Windows, too, as the trailing
dot is kept for URLs and absolute Windows paths.
> I have no expertise on how to deal with these path issues, but it sounds
> like this dot-stripping is done too early, i.e. you'd want to first let
> the Git part handle the URL concatenation and stuff and only at the end
> when it comes to using the path it should get the Windows treatment?
Git has no chance to fix this, as the Git executable (thanks to *not*
using the POSIX emulation layer) gets handed a Windows path without the
trailing dot when called from the Bash.
The same, obviously, goes for `git submodule-helper`: it is not using the
POSIX emulation layer, and therefore that layer converts the paths before
executing said subcommand.
And we cannot easily change the behavior of the MSYS2 runtime, as that
would affect too many other users, most likely breaking the use case that
required the stripping of the trailing dot in the first place.
So I fear that we have to live with the fact that the bug you fixed just
hid a bug on Windows, and that we have to either skip the tests or change
them in the way I proposed.
Or we change the tests to work on a URL instead of a Unixy path.
Opinions?
Ciao,
Dscho
^ permalink raw reply
* Re: Re* [PATCH v3 05/25] sequencer: eventually release memory allocated for the option values
From: Johannes Schindelin @ 2016-10-13 10:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, René Scharfe
In-Reply-To: <xmqqk2dd4efs.fsf_-_@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 12 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Tue, 11 Oct 2016, Junio C Hamano wrote:
> >
> >> The only reason why the OPT_STRDUP appeared convenient was because
> >> options[] element happened to use a field in the structure directly.
> >> The patch under discussion does an equivalent of
> >>
> >> app.x_field = xstrdup_or_null(opt_x);
> >
> > Oh, that xstrdup_or_null() function slipped by me. My local patches use it
> > now.
>
> It has slipped many people ;-)
Thanks, I feel better now ;-)
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 13/25] sequencer: prepare for rebase -i's commit functionality
From: Johannes Schindelin @ 2016-10-13 10:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, Michael Haggerty
In-Reply-To: <xmqq60ox5wxb.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 12 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> Hmph, didn't we recently add parse_key_value_squoted() to build
> >> read_author_script() in builtin/am.c on top of it, so that this
> >> piece of code can also take advantage of and share the parser?
> >
> > I already pointed out that the author-script file may *not* be quoted.
>
> I think my puzzlement comes from here. What makes it OK for "am" to
> expect the contents of author-script file to be quoted but it is not
> OK to expect the same here? What makes it not quoted for _this_
> reader, in other words?
The `git am` command is inherently *not* interactive, while the
interactive rebase, well, is.
As such, we must assume that enterprisey users did come up with scripts
that edit, or create, author-script files, and exploited the fact that the
interactive rebase previously sourced them.
Come to think of it, there is a bigger problem here, as users might have
abused the author-script to execute commands in rebase -i's own context.
Not sure we can do anything about that.
But the point stands, if anybody used unquoted, or differently quoted,
values in author-script, we should at least attempt within reason to
support that.
> I am not sure what you meant by "nominally related", but the purpose
> of the author-script in these two codepaths is the same, isn't it?
The purpose is, but the means aren't. As I pointed out above, the
interactive rebase is inherently much more interactive, and needs to be
much more forgiving in its input, than `git am`.
> Somebody leaves the author information from the source (either from
> an e-mailed patch or an existing commit), so that a later step can
> use that pieces of information left in the file when (re)creating a
> commit to record the tree made by using pieces of information from
> the source.
>
> Are our use in the author-script in these two codepaths _already_
> inconsistent? IOW, "am" never writes malformed unquoted values,
> while the sequencer writes out in a way that is randomly quoted or
> not quoted, iow, if you fed such an author-file to "am", it wouldn't
> understand it?
We heed Postel's Law: what the sequencer writes is in a very strict
format, but what the sequencer accepts need not be.
> I fully support your position to use different codepaths, if the
> file that has the same name and that is used for the same purpose
> uses different format in these two separate codepaths and the users
> already expect them to be different. We obviously need to have two
> separate parsers.
Well, traditionally we *do* have separate parsers. I do not say that we
need to keep that, but given what I said above, it might not be a bad idea
to keep the lenient parser required by `git rebase -i` separate from the
one used by `git am` so that the latter can be faster (by making
assumptions the other parser cannot).
> But if that is not the case, IOW, if "am"'s author-script shares the
> same issue (i.e. "'am' initially writes the file properly quoted,
> but this or that can happen to change its quoting and we need to
> read from such a file"), then perhaps sharing needs to happen the
> other way around? This patch may prepare "rebase -i" side for the
> "this or that" (I still do not know what they are) to allow the
> resulting file read correctly, but the same "this or that" can break
> what "am" has used and is in use there if that is the case, no?
>
> What makes it OK for "am" to expect the contents of author-script
> file to be quoted but it is not OK to expect the same here? What
> makes it not quoted for _this_ reader, and doesn't "am" share the
> same issue?
The fact that `git am` is *non-interactive*.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 12/25] sequencer: remember the onelines when parsing the todo file
From: Johannes Schindelin @ 2016-10-13 10:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqbmyp5ycw.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 12 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> > + const char *arg;
> >> > + int arg_len;
> >> > size_t offset_in_buf;
> >>
> >> micronit: you can make it to size_t and lose the cast below, no?
> >
> > No. The primary users of arg_len call a printf() style function with %.*s,
> > expecting an int. So your suggestion would lose one cast, but introduce at
> > least four casts in return.
>
> Actually my point was not the number of casts required, but more
> about using the correct type to store things. Granted, I do not
> expect each of the lines would ever get too long to exceed "int"
> (but fit in "size_t") in practice, and from that point of view, one
> may be able to argue that "int" and "size_t" are both correct types,
> but that argument applies equally to offset_in_buf, so...
You cannot make it a size_t without changing the printf() standard to
expect size_t for %.*s, because that is the intended usage of that field
in these here patches.
Ciao,
Dscho
^ permalink raw reply
* Re: interactive rebase should better highlight the not-applying commit
From: Johannes Schindelin @ 2016-10-13 10:40 UTC (permalink / raw)
To: Joshua N Pritikin; +Cc: git@vger.kernel.org
In-Reply-To: <20161012170207.lapdv5h5aws4k4pw@droplet>
Hi Joshua,
On Wed, 12 Oct 2016, Joshua N Pritikin wrote:
> On Wed, Oct 12, 2016 at 06:24:37PM +0200, Johannes Schindelin wrote:
>
> > But maybe I read it all wrong and you do want to make this happen
> > yourself, and you simply want a little advice how to go about it?
>
> Ugh, if you insist.
I don't. If you want that feature to see the light of day, you should
insist yourself ;-)
> > > On Tue, Oct 11, 2016 at 02:25:19PM -0700, Stefan Beller wrote:
> > > > On Tue, Oct 11, 2016 at 12:07 PM, Joshua N Pritikin <jpritikin@pobox.com> wrote:
> > > > However IIUC currently rebase is completely rewritten/ported to C
> > > > where it is easier to add color support as we do have some color
> > > > support in there already.
> > >
> > > Sounds great. Is there a beta release that I can try out?
> >
> > There is no release as such, unless you count Git for Windows v2.10.0.
>
> Nope, that doesn't count. ;-)
Sometimes honesty goes too far. You basically told me that what I work on
does not count. That does not exactly curry my favor.
> > But you can try the `interactive-rebase` branch of
> > https://github.com/dscho/git; please note, though, that my main aim
> > was to be as faithful as possible in the conversion (modulo speed, of
> > course).
>
> Hm OK
>
> > > Sometimes I do a rebase to fix some tiny thing 10-15 commits from HEAD.
> > > Maybe only 1 file is affected and there are no merge conflicts, but when
> > > rebase reapplies all the commits, the timestamps of lots of unmodified
> > > files change even though they are unmodified compared to before the
> > > rebase.
> >
> > Well, they *were* modified, right?
>
> Were they? Isn't that just an artefact of the implementation?
Yes, they were modified, as the todo script you saved for the interactive
rebase to perform told it to cherry-pick those changes. That is a worktree
operation, performing on files, not a repository operation working on
objects in Git's database.
> > A workaround would be to create a new worktree using the awesome `git
> > worktree` command, perform the rebase there (on an unnamed branch --
> > AKA "detached HEAD", no relation to Helloween), and then come back to
> > the original worktree and reset --hard to the new revision. That reset
> > would detect that there are actually no changes required to said
> > files.
>
> What would be the problem with doing this by default? Or could it be a
> configuration option that can be enabled?
It could definitely be a new feature that is triggered by a new (opt-in)
configuration option.
It cannot be on by default, at least not in the short run, because those
cherry-picks can fail with merge conflicts and power users of the
interactive rebase expect those conflicts to show in the current worktree.
Ciao,
Johannes
^ permalink raw reply
* Re: problem with git worktree and git svn
From: Duy Nguyen @ 2016-10-13 10:29 UTC (permalink / raw)
To: Eric Wong; +Cc: Mathieu Arnold, Stefan Beller, Git Mailing List
In-Reply-To: <20161013015233.GA18001@whir>
On Thu, Oct 13, 2016 at 8:52 AM, Eric Wong <e@80x24.org> wrote:
> +sub svn_dir {
> + my $git_dir = scalar @_ ? $_[0] : $ENV{GIT_DIR};
> + my $common = $ENV{GIT_COMMON_DIR} || "$git_dir/commondir";
> + $git_dir .= '/'.::file_to_s($common) if -e $common;
> + my $svn_dir = $git_dir . '/svn';
> + $svn_dir =~ tr!/!/!s;
> + $svn_dir;
> +}
If this is shell script, this function could be just
svn_dir() {
git rev-parse --git-path svn
}
which should give you correct path in either single or multi-worktree
context and you don't need to bother with details like
$GIT_COMMON_DIR. But I don't know how Perl bindings are implemented, I
don't know if we have something similar (or easy to add it, like
Git::git_path()).
I don't know much about git-svn, but from the look of it I agree
replacing $ENV{GIT_DIR}/svn with svn_dir() should fix it, assuming
that you don't hard code $ENV{GIT_DIR}/blahblah somewhere else. I
don't see any other potential problems (from multi-worktree
perspective).
--
Duy
^ permalink raw reply
* Re: [PATCH] worktree: allow the main brach of a bare repository to be checked out
From: Duy Nguyen @ 2016-10-13 10:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: Dennis Kaarsemaker, Git Mailing List, Michael Tutty,
Michael Rappazzo
In-Reply-To: <xmqqbmyp4d28.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 13, 2016 at 1:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>>
>>> OK, so here it is as a proper patch.
>
> Here is what I queued. Duy, what do you think? It seems OK to me.
Ack. Thanks both.
--
Duy
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Vegard Nossum @ 2016-10-13 7:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Quentin Casasnovas, Shawn Pearce, Jeff King,
Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqpon5190s.fsf@gitster.mtv.corp.google.com>
On 10/13/2016 12:45 AM, Junio C Hamano wrote:
> Vegard Nossum <vegard.nossum@oracle.com> writes:
>
>> A closer inspection reveals the problem to really be that this is an
>> extremely hot path with more than -- holy cow -- 4,106,756,451
>> iterations on the 'packed_git' list for a single 'git fetch' on my
>> repository. I'm guessing the patch above just made the inner loop
>> ever so slightly slower.
>
> Very plausible, and this ...
>
>> My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
>> and 4 tmp_pack_* files).
>
> ... may explain why nobody else has seen a difference.
>
> Is there a reason why your repository has that many pack files? Is
> automatic GC not working for some reason?
Oops. I disabled gc a while ago; one reason I did that is that it takes
a long time to run and it has a tendency to kick in at the worst time. I
guess I should really put it in cron then.
I'm not sure if this is related, but I also had a problem with GitPython
and large pack files in the past (" ValueError: Couldn't obtain fanout
table or warning: packfile ./objects/pack/....pack cannot be accessed")
and I have pack.packSizeLimit set to 512m to fix that.
Although the whole repo is 17G so I guess it shouldn't be necessary to
have that many pack files.
Will try Jeff's patch, then a gc. Thanks!
Vegard
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Vegard Nossum @ 2016-10-13 9:04 UTC (permalink / raw)
To: Jeff King
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161012234753.tbqhuc7qdyklpfzv@sigill.intra.peff.net>
On 10/13/2016 01:47 AM, Jeff King wrote:
> On Wed, Oct 12, 2016 at 07:18:07PM -0400, Jeff King wrote:
>
>> Also, is it possible to make the repository in question available? I
>> might be able to reproduce based on your description, but it would save
>> time if I could directly run gdb on your example.
I won't be able to make the repository available, sorry.
> I tried this by making a bunch of packs in linux.git (my standard "this
> is pretty big" repo), like so:
>
> for i in $(seq 1000); do
> git rev-list --objects HEAD~$((i+1))..HEAD~$i |
> git pack-objects --delta-base-offset .git/objects/pack/pack
> done
>
> and then doing a 25,000-object fetch from upstream (no significance to
> the number; that's just how far behind upstream I happened to be).
>
> However, I didn't notice any regression. In fact, it was much _slower_
> than v1.9.0, because that older version didn't have threaded index-pack.
>
> If you can't share the repo directly, can you tell us more about your
> fetch? How many objects are in your repository? How many objects are
> fetched? How many refs are there on the remote side?
The fetch doesn't actually get anything from the remote as everything is
already up to date (that makes the 2m40s times even more frustrating in
a way :-)). Here's count-objects:
$ git count-objects -v
warning: garbage found: .git/objects/pack/tmp_pack_pAZcu4
warning: garbage found: .git/objects/pack/tmp_pack_KhzrrI
warning: garbage found: .git/objects/pack/tmp_pack_mycfro
warning: garbage found: .git/objects/pack/tmp_pack_2kxKOn
count: 51609
size: 288768
in-pack: 23902336
packs: 1044
size-pack: 16588157
prune-packable: 48628
garbage: 4
size-garbage: 84792
There are some 20k refs on the remote, closer to 25k locally.
I'll try to get a profile (without your patch) before doing a gc run.
Vegard
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Vegard Nossum @ 2016-10-13 7:20 UTC (permalink / raw)
To: Jeff King
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161012230143.5kxcmtityaasra5j@sigill.intra.peff.net>
On 10/13/2016 01:01 AM, Jeff King wrote:
> On Thu, Oct 13, 2016 at 12:30:52AM +0200, Vegard Nossum wrote:
>
>> However, the commit found by 'git blame' above appears just fine to me,
>> I haven't been able to spot a bug in it.
>>
>> A closer inspection reveals the problem to really be that this is an
>> extremely hot path with more than -- holy cow -- 4,106,756,451
>> iterations on the 'packed_git' list for a single 'git fetch' on my
>> repository. I'm guessing the patch above just made the inner loop
>> ever so slightly slower.
>>
>> My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
>> and 4 tmp_pack_* files).
>
> Yeah. I agree that the commit you found makes the check a little more
> expensive, but I think the root of the problem is calling
> prepare_packed_git_one many times. This _should_ happen once for each
> pack at program startup, and possibly again if we need to re-scan the
> pack directory to account for racing with a simultaneous repack.
>
> The latter is generally triggered when we fail to look up an object we
> expect to exist. So I'd suspect 45e8a74 (has_sha1_file: re-check pack
> directory before giving up, 2013-08-30) is playing a part. We dealt with
> that to some degree in 0eeb077 (index-pack: avoid excessive re-reading
> of pack directory, 2015-06-09), but it would not surprise me if there is
> another spot that needs similar treatment.
>
> Does the patch below help?
Yes, ~2m10s -> ~1m25s when I test a git fetch this morning (the other
variation in time may be due to different CPU usage by other programs,
but I ran with/without the patch multiple times and the difference is
consistent).
Thanks,
Vegard
^ permalink raw reply
* Re: [PATCH] merge-base: handle --fork-point without reflog
From: Stepan Kasal @ 2016-10-13 6:34 UTC (permalink / raw)
To: Jeff King; +Cc: John Keeping, git
In-Reply-To: <20161012201040.pyrp6bktz3fgmqzn@sigill.intra.peff.net>
Hello,
thank you for this nice and quick fix of this corner case!
Stepan
^ permalink raw reply
* [PATCH] parse_mailboxes: accept extra text after <...> address
From: Matthieu Moy @ 2016-10-13 5:47 UTC (permalink / raw)
To: gitster
Cc: git, Larry Finger, Jeff King, Mathieu Lienard--Mayor,
Remi Lespinet, Matthieu Moy
In-Reply-To: <vpqmvi8n71g.fsf@anie.imag.fr>
The test introduced in this commit succeeds without the patch to Git.pm
if Mail::Address is installed, but fails otherwise because our in-house
parser does not accept any text after the email address. They succeed
both with and without Mail::Address after this commit.
Mail::Address accepts extra text and considers it as part of the name,
iff the address is surrounded with <...>. The implementation mimics
this behavior as closely as possible.
This mostly restores the behavior we had before b1c8a11 (send-email:
allow multiple emails using --cc, --to and --bcc, 2015-06-30), but we
keep the possibility to handle comma-separated lists.
Reported-by: Larry Finger <Larry.Finger@lwfinger.net>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
perl/Git.pm | 13 +++++++------
t/t9001-send-email.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index ce7e4e8da394..ca769246216c 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -879,6 +879,7 @@ sub parse_mailboxes {
# divide the string in tokens of the above form
my $re_token = qr/(?:$re_quote|$re_word|$re_comment|\S)/;
my @tokens = map { $_ =~ /\s*($re_token)\s*/g } @_;
+ my $end_of_addr_seen = 0;
# add a delimiter to simplify treatment for the last mailbox
push @tokens, ",";
@@ -888,10 +889,10 @@ sub parse_mailboxes {
if ($token =~ /^[,;]$/) {
# if buffer still contains undeterminated strings
# append it at the end of @address or @phrase
- if (@address) {
- push @address, @buffer;
- } else {
+ if ($end_of_addr_seen) {
push @phrase, @buffer;
+ } else {
+ push @address, @buffer;
}
my $str_phrase = join ' ', @phrase;
@@ -915,16 +916,16 @@ sub parse_mailboxes {
push @addr_list, $str_mailbox if ($str_mailbox);
@phrase = @address = @comment = @buffer = ();
+ $end_of_addr_seen = 0;
} elsif ($token =~ /^\(/) {
push @comment, $token;
} elsif ($token eq "<") {
push @phrase, (splice @address), (splice @buffer);
} elsif ($token eq ">") {
+ $end_of_addr_seen = 1;
push @address, (splice @buffer);
- } elsif ($token eq "@") {
+ } elsif ($token eq "@" && !$end_of_addr_seen) {
push @address, (splice @buffer), "@";
- } elsif ($token eq ".") {
- push @address, (splice @buffer), ".";
} else {
push @buffer, $token;
}
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index b3355d2c7016..3dc4a3454d22 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -140,6 +140,35 @@ test_expect_success $PREREQ 'Verify commandline' '
test_cmp expected commandline1
'
+test_expect_success $PREREQ 'setup expect for cc trailer' "
+cat >expected-cc <<\EOF
+!recipient@example.com!
+!author@example.com!
+!one@example.com!
+!two@example.com!
+!three@example.com!
+!four@example.com!
+!five@example.com!
+EOF
+"
+
+test_expect_success $PREREQ 'cc trailer with various syntax' '
+ test_commit cc-trailer &&
+ test_when_finished "git reset --hard HEAD^" &&
+ git commit --amend -F - <<-EOF &&
+ Test Cc: trailers.
+
+ Cc: one@example.com
+ Cc: <two@example.com> # this is part of the name
+ Cc: <three@example.com>, <four@example.com> # not.five@example.com
+ Cc: "Some # Body" <five@example.com> [part.of.name.too]
+ EOF
+ clean_fake_sendmail &&
+ git send-email -1 --to=recipient@example.com \
+ --smtp-server="$(pwd)/fake.sendmail" &&
+ test_cmp expected-cc commandline1
+'
+
test_expect_success $PREREQ 'setup expect' "
cat >expected-show-all-headers <<\EOF
0001-Second.patch
--
2.10.0.rc0.1.g07c9292
^ permalink raw reply related
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-13 5:37 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Larry Finger, Mathieu Lienard--Mayor,
Remi Lespinet, git
In-Reply-To: <20161012231321.ar2csdmvhdya2q7r@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 2. Drop everything after the trailing ">". This gives a valid rfc2822
> cc, and people can pick the "# 4.8" from the cc line in the body.
That would work for me, but it's inconsistent with Mail::Address and I'd
really like to avoid having a behavior depending on libraries installed.
Plus, consistency with Mail::Address gives us consistency with any other
program using Mail::Address.
> 3. Rewrite
>
> A <B@C> D
>
> into
>
> A D <B@C>
>
> regardless of what is in "D". This retains the information in the
> rfc2822 cc.
There's another one I considered:
4. Consider '#.*' as a comment when parsing trailers (but not in
other places where we parse addresses)
This is much harder to define properly because of
Cc: "Foo # Bar" <email@example.com>
=> we'd need to handle the ".*" syntax when stripping comments. And
again, that wouldn't be consistent with Mail::Address.
So, I ended up implementing 3., which actually isn't hard, and gives
code IMHO cleaner than it used to be.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-13 5:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Larry Finger, Jeff King, Mathieu Lienard--Mayor, Remi Lespinet,
git
In-Reply-To: <xmqqtwch2srj.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> People write things like these
>
> Cc: Stable <stable@vger.kernel.org> # 4.8
> Cc: Stable <stable@vger.kernel.org> [4.8+]
>
> in the trailer part in the body of the message. Are these lines
> meant to be usable if they appear as Cc: headers of an outgoing
> piece of e-mail as-is?
I think this is not the right question. The relevant one is: should
these lines be accepted by git and turned into something usable if they
appear as Cc: headers of an outgoing piece of e-mail?
I.e. "Be liberal in what you accept, and conservative in what you send".
If you have Mail::Address installed, this is already possible. With
pre-2.6, I did not re-test, but I think it was using the addresses
as-is, which probably worked but AFAICT created non-RCF-compliant
emails.
> "error: unable to extract a valid address from:" is followed by
>
> Stable <stable@vger.kernel.org#4.8>
> Stable <stable@vger.kernel.org[4.8+]>
>
> which is not ideal.
I'd actually even say "broken" ;-). If we decide to reject these, we
should at least give a sensible error message.
> If I were to issue a decree, I would say that people should stop
> suggesting to put RFC-bogus things on the Cc: line. As you
> mentioned, things like:
>
> Cc: Stable (4.8+) <stable@vger.kernel.org>
> Cc: "Stable 4.8+" <stable@vger.kernel.org>
>
> are perfectly readable alternative that are still RFC-kosher.
I do support those, but if there's an established tradition of using
# ... trailer, then I don't think we should be the ones forcing it to
stop.
> Things may have appeared to work by accident, and things may still
> work by accident, depending on the vintage and availability of
> Mail::Address package (which seems to be the case), but it is not
> worth risking random breakages that depends on what other people
> use in the first place, no?
The "depending on the availability of Mail::Address" is what bothers me
most. Suppose we make a strong statement here that this # 4.8 should
stop. Then some users will listen to that statements, but others won't
read the thread, test with their own git that it works, and recommend it
to users for whom it doesn't.
> That is, even though people seem to expect "send-email --cc" to
> somehow be able to judge that " # 4.8" and " [4.8+]" are cruft not
> meant as part of a valid address, I do not think it is a realistic
> expectation. How would it know "Cc: Stable <add@re.ss> 4.8, 4.9"
> has garbage " 4.8, 4.9" that needs to be stripped, while "Cc: Stable
> <add@re.ss> 4.8, torvalds@linux-foundation.org" has two valid
> addresses that need to be CC'ed and " 4.8" is the only thing that is
> unwanted?
We clearly can't guess, but we can be consistent with Mail::Address, so
that git's behavior depends less on its availability.
Patch follows doing that.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 1/2] Feature Request: user defined suffix for temp files created by git-mergetool
From: David Aguilar @ 2016-10-13 5:13 UTC (permalink / raw)
To: Josef Ridky; +Cc: git
In-Reply-To: <1911899288.2172724.1475757782111.JavaMail.zimbra@redhat.com>
On Thu, Oct 06, 2016 at 08:43:02AM -0400, Josef Ridky wrote:
> This is the first of two variant for request to add option to change
> suffix of name of temporary files generated by git mergetool. This
> change is requested for cases, when is git mergetool used for local
> comparision between two version of same package during package rebase.
>
> Signed-off-by: Josef Ridky <jridky@redhat.com>
> ---
> Documentation/git-mergetool.txt | 7 ++++++-
> git-mergetool.sh | 23 ++++++++++++++++++-----
> 2 files changed, 24 insertions(+), 6 deletions(-)
While I do like that this variant only uses a single flag, I was
curious whether it would make sense for us to change our
defaults to OLD/NEW? I'm thinking "no" since it's totally legit
to merge "old" branches so I'll stop there.
What I really wanted to mention was...
If the patch does not update t/t7610-mergetool.sh then there is
no guarantee that my clumsy fingers won't break the new feature
in the future ;-) So please make sure to update the tests once
we decide on the final direction. It makes sense we wouldn't
want to update them just yet (in this patch) since this is still
RFC, but the final one should include it.
I'm still leaning towards environment variables personally,
and would have no qualms against taking a patch that teaches it
to support environment variables as long as it adds a test case
for the new feature.
Thanks for sticking with it, Josef!
cheers,
--
David
^ permalink raw reply
* Re: [PATCH v2 2/2] Feature Request: user defined suffix for temp files created by git-mergetool
From: David Aguilar @ 2016-10-13 4:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Josef Ridky
In-Reply-To: <xmqqpon54fe5.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 10:59:46AM -0700, Junio C Hamano wrote:
> Josef Ridky <jridky@redhat.com> writes:
>
> > This is update of the second variant for request to add option to change
> > suffix of name of temporary files generated by git mergetool. This
> > change is requested for cases, when is git mergetool used for local
> > comparison between two version of same package during package rebase.
> >
> > Signed-off-by: Josef Ridky <jridky@redhat.com>
> > ---
>
> David, what do you think?
>
> I don't think you were ever CC'ed on any of the messages in
> this thread, and I don't think you've commented on the topic. The
> thread begins here:
>
> https://public-inbox.org/git/1329039097.128066.1475476591437.JavaMail.zimbra@redhat.com/
>
>
> In any case, I suggest update to the log message to something like
> this:
>
> Subject: mergetool: allow custom naming for temporary files
>
> A front-end program that is spawned by "git mergetool" is given
> three temporary files (e.g. it may get "x_LOCAL.txt",
> "x_REMOTE.txt", and "x_BASE.txt" while merging "x.txt").
>
> Custom wrappers to "git mergetool" benefits if they are allowed
> to rename these hardcoded suffixes to match the workflow they
> implement. For example, they may be used to compare and merge
> two versions that is available locally, and OLD/NEW may be more
> appropriate than LOCAL/REMOTE in such a context.
>
> primarily because "the second variant" is meaningless thing to say
> in our long term history, when the first variant never was recorded
> there. Josef may want to elaborate more on the latter paragraph.
I do see why this can be helpful but what makes me reluctant is,
> > +'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>] [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
We're parking on 4 new flags that are really all about changing
one small aspect of the program.
I don't typically like going overboard with environment
variables but for this use case they seem to be a good fit.
It's already been expressed that the intention is for these to
be supplied by some other tool, and environment variables have
the nice property that you can update all your tools without
needing to touch anything except the environment.
Another nice thing is that the the user can choose to set it
globally, or to set it local to specific tools.
Another reason why I think it's a good fit is,
> +# Can be changed by user
> +LOCAL_NAME='LOCAL'
> +BASE_NAME='BASE'
> +BACKUP_NAME='BACKUP'
> +REMOTE_NAME='REMOTE'
These lines would become simply,
LOCAL_NAME=${GIT_MERGETOOL_LOCAL_NAME:-LOCAL}
BASE_NAME=${GIT_MERGETOOL_BASE_NAME:-BASE}
BACKUP_NAME=${GIT_MERGETOOL_BACKUP_NAME:-BACKUP}
REMOTE_NAME=${GIT_MERGETOOL_REMOTE_NAME:-REMOTE}
and we wouldn't need any other change besides this part:
> - BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
> - LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
> - REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
> - BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
> + BACKUP="$MERGETOOL_TMPDIR/${BASE}_${BACKUP_NAME}_$$$ext"
> + LOCAL="$MERGETOOL_TMPDIR/${BASE}_${LOCAL_NAME}_$$$ext"
> + REMOTE="$MERGETOOL_TMPDIR/${BASE}_${REMOTE_NAME}_$$$ext"
> + BASE="$MERGETOOL_TMPDIR/${BASE}_${BASE_NAME}_$$$ext"
Then, just update the documentation to mention the environment
variables instead of the flags and we're all set.
We also won't need this part if we go down that route:
> +# sanity check after parsing command line
> +case "" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/remote/base/backup to empty."
> + ;;
> +esac
> +
> +case "$LOCAL_NAME" in
> +"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --remote/base/backup to same as --local."
> + ;;
> +esac
> +
> +case "$REMOTE_NAME" in
> +"$LOCAL_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/base/backup to same as --remote."
> + ;;
> +esac
> +
> +case "$BASE_NAME" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/remote/backup to same as --base."
> + ;;
> +esac
> +
> +case "$BACKUP_NAME" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME")
> + die "You cannot set any of --local/remote/base to same as --backup."
> + ;;
> +esac
What do you think?
--
David
^ permalink raw reply
* Re: problem with git worktree and git svn
From: Eric Wong @ 2016-10-13 1:52 UTC (permalink / raw)
To: Mathieu Arnold; +Cc: Duy Nguyen, Stefan Beller, git
In-Reply-To: <CAGZ79kZo5W1r0s26G3foB7caP6+u66mdzqzyneqXBX_B7A0RKg@mail.gmail.com>
Stefan Beller <sbeller@google.com> wrote:
> On Wed, Oct 12, 2016 at 7:45 AM, Mathieu Arnold <mat@freebsd.org> wrote:
>
> > I discovered git worktree earlier this week, and I found it a great
> > asset to be able to have more than one branch of my worktree accessible
> > at the same time...
> >
> > Anyway, back to my problem, the way git-svn works, is that it looks for
> > a directory named "svn" in its gitdir and if it is not present, decide
> > the repository is using git-svn version 1 (whatever that is) and goes to
> > parse all the revisions to recreate the svn directory.
> > So I can only use git svn commands in my main worktree, the one with the
> > real gitdir.
Right, I haven't updated git-svn to be worktree-aware, yet, but
a work-in-progress is below
> > To fix that, all I had to do is to add a symlink named svn in each
> > worktree's gitdir and pointing to ../../svn.
>
> For some definition of fix. ;)
> Sure it fixes your local setup now, but would we want to use that as well here?
> My gut reaction:
>
> * not all platforms know symlinks
> * IIRC there is some worktree magic that tells you the "main" dir,
> so if that was used in git-svn instead it should "just work".
>
> >
> > I think all that needs to happen is that when adding a new worktree, if
> > the main git directory has a "svn" directory, add a symlink to it in the
> > worktree's gitdir.
I'm fairly sure the worktree C code should not care about how
git-svn works, but rather git-svn should be made aware of
worktree bits...
I haven't studied worktrees much nor do I use git-svn much,
but the following seems to work for fetch/rebase/dcommit:
------------8<-----------
Subject: [PATCH] git-svn: WIP worktree awareness
---
perl/Git/SVN.pm | 29 ++++++++++++++++++++---------
perl/Git/SVN/Migration.pm | 23 ++++++++++++-----------
2 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 018beb8..267cc09 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -807,10 +807,20 @@ sub get_fetch_range {
(++$min, $max);
}
+sub svn_dir {
+ my $git_dir = scalar @_ ? $_[0] : $ENV{GIT_DIR};
+ my $common = $ENV{GIT_COMMON_DIR} || "$git_dir/commondir";
+ $git_dir .= '/'.::file_to_s($common) if -e $common;
+ my $svn_dir = $git_dir . '/svn';
+ $svn_dir =~ tr!/!/!s;
+ $svn_dir;
+}
+
sub tmp_config {
my (@args) = @_;
- my $old_def_config = "$ENV{GIT_DIR}/svn/config";
- my $config = "$ENV{GIT_DIR}/svn/.metadata";
+ my $svn_dir = svn_dir();
+ my $old_def_config = "$svn_dir/config";
+ my $config = "$svn_dir/.metadata";
if (! -f $config && -f $old_def_config) {
rename $old_def_config, $config or
die "Failed rename $old_def_config => $config: $!\n";
@@ -1671,7 +1681,7 @@ sub tie_for_persistent_memoization {
return if $memoized;
$memoized = 1;
- my $cache_path = "$ENV{GIT_DIR}/svn/.caches/";
+ my $cache_path = svn_dir() . '/.caches/';
mkpath([$cache_path]) unless -d $cache_path;
my %lookup_svn_merge_cache;
@@ -1712,7 +1722,7 @@ sub tie_for_persistent_memoization {
sub clear_memoized_mergeinfo_caches {
die "Only call this method in non-memoized context" if ($memoized);
- my $cache_path = "$ENV{GIT_DIR}/svn/.caches/";
+ my $cache_path = svn_dir() . '/.caches/';
return unless -d $cache_path;
for my $cache_file (("$cache_path/lookup_svn_merge",
@@ -2446,12 +2456,13 @@ sub _new {
"refs/remotes/$prefix$default_ref_id";
}
$_[1] = $repo_id;
- my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
+ my $svn_dir = svn_dir();
+ my $dir = "$svn_dir/$ref_id";
- # Older repos imported by us used $GIT_DIR/svn/foo instead of
- # $GIT_DIR/svn/refs/remotes/foo when tracking refs/remotes/foo
+ # Older repos imported by us used $svn_dir/foo instead of
+ # $svn_dir/refs/remotes/foo when tracking refs/remotes/foo
if ($ref_id =~ m{^refs/remotes/(.+)}) {
- my $old_dir = "$ENV{GIT_DIR}/svn/$1";
+ my $old_dir = "$svn_dir/$1";
if (-d $old_dir && ! -d $dir) {
$dir = $old_dir;
}
@@ -2461,7 +2472,7 @@ sub _new {
mkpath([$dir]);
my $obj = bless {
ref_id => $ref_id, dir => $dir, index => "$dir/index",
- config => "$ENV{GIT_DIR}/svn/config",
+ config => "$svn_dir/config",
map_root => "$dir/.rev_map", repo_id => $repo_id }, $class;
# Ensure it gets canonicalized
diff --git a/perl/Git/SVN/Migration.pm b/perl/Git/SVN/Migration.pm
index cf6ffa7..887fd93 100644
--- a/perl/Git/SVN/Migration.pm
+++ b/perl/Git/SVN/Migration.pm
@@ -45,6 +45,7 @@ use Git qw(
command_output_pipe
command_close_pipe
);
+use Git::SVN;
sub migrate_from_v0 {
my $git_dir = $ENV{GIT_DIR};
@@ -82,7 +83,7 @@ sub migrate_from_v1 {
my $git_dir = $ENV{GIT_DIR};
my $migrated = 0;
return $migrated unless -d $git_dir;
- my $svn_dir = "$git_dir/svn";
+ my $svn_dir = Git::SVN::svn_dir($git_dir);
# just in case somebody used 'svn' as their $id at some point...
return $migrated if -d $svn_dir && ! -f "$svn_dir/info/url";
@@ -100,24 +101,23 @@ sub migrate_from_v1 {
next unless -f "$git_dir/$x/info/url";
my $u = eval { ::file_to_s("$git_dir/$x/info/url") };
next unless $u;
- my $dn = dirname("$git_dir/svn/$x");
+ my $dn = dirname("$svn_dir/$x");
mkpath([$dn]) unless -d $dn;
if ($x eq 'svn') { # they used 'svn' as GIT_SVN_ID:
- mkpath(["$git_dir/svn/svn"]);
+ mkpath(["$svn_dir/svn"]);
print STDERR " - $git_dir/$x/info => ",
- "$git_dir/svn/$x/info\n";
- rename "$git_dir/$x/info", "$git_dir/svn/$x/info" or
+ "$svn_dir/$x/info\n";
+ rename "$git_dir/$x/info", "$svn_dir/$x/info" or
croak "$!: $x";
# don't worry too much about these, they probably
# don't exist with repos this old (save for index,
# and we can easily regenerate that)
foreach my $f (qw/unhandled.log index .rev_db/) {
- rename "$git_dir/$x/$f", "$git_dir/svn/$x/$f";
+ rename "$git_dir/$x/$f", "$svn_dir/$x/$f";
}
} else {
- print STDERR " - $git_dir/$x => $git_dir/svn/$x\n";
- rename "$git_dir/$x", "$git_dir/svn/$x" or
- croak "$!: $x";
+ print STDERR " - $git_dir/$x => $svn_dir/$x\n";
+ rename "$git_dir/$x", "$svn_dir/$x" or croak "$!: $x";
}
$migrated++;
}
@@ -139,9 +139,10 @@ sub read_old_urls {
push @dir, $_;
}
}
+ my $svn_dir = Git::SVN::svn_dir();
foreach (@dir) {
my $x = $_;
- $x =~ s!^\Q$ENV{GIT_DIR}\E/svn/!!o;
+ $x =~ s!^\Q$svn_dir\E/!!o;
read_old_urls($l_map, $x, $_);
}
}
@@ -150,7 +151,7 @@ sub migrate_from_v2 {
my @cfg = command(qw/config -l/);
return if grep /^svn-remote\..+\.url=/, @cfg;
my %l_map;
- read_old_urls(\%l_map, '', "$ENV{GIT_DIR}/svn");
+ read_old_urls(\%l_map, '', Git::SVN::svn_dir());
my $migrated = 0;
require Git::SVN;
--
EW
^ permalink raw reply related
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-12 23:47 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161012231807.syockv2emrsjf55r@sigill.intra.peff.net>
On Wed, Oct 12, 2016 at 07:18:07PM -0400, Jeff King wrote:
> Also, is it possible to make the repository in question available? I
> might be able to reproduce based on your description, but it would save
> time if I could directly run gdb on your example.
I tried this by making a bunch of packs in linux.git (my standard "this
is pretty big" repo), like so:
for i in $(seq 1000); do
git rev-list --objects HEAD~$((i+1))..HEAD~$i |
git pack-objects --delta-base-offset .git/objects/pack/pack
done
and then doing a 25,000-object fetch from upstream (no significance to
the number; that's just how far behind upstream I happened to be).
However, I didn't notice any regression. In fact, it was much _slower_
than v1.9.0, because that older version didn't have threaded index-pack.
If you can't share the repo directly, can you tell us more about your
fetch? How many objects are in your repository? How many objects are
fetched? How many refs are there on the remote side?
-Peff
^ permalink raw reply
* [PATCH v2 6/6] trailer: support values folded to multiple lines
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Currently, interpret-trailers requires that a trailer be only on 1 line.
For example:
a: first line
second line
would be interpreted as one trailer line followed by one non-trailer line.
Make interpret-trailers support RFC 822-style folding, treating those
lines as one logical trailer.
---
Documentation/git-interpret-trailers.txt | 7 +-
t/t7513-interpret-trailers.sh | 139 +++++++++++++++++++++++++++++++
trailer.c | 22 +++--
3 files changed, 160 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index c480da6..cfec636 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -57,11 +57,12 @@ minus signs start the patch part of the message.
When reading trailers, there can be whitespaces before and after the
token, the separator and the value. There can also be whitespaces
-inside the token and the value.
+inside the token and the value. The value may be split over multiple lines with
+each subsequent line starting with whitespace, like the "folding" in RFC 822.
Note that 'trailers' do not follow and are not intended to follow many
-rules for RFC 822 headers. For example they do not follow the line
-folding rules, the encoding rules and probably many other rules.
+rules for RFC 822 headers. For example they do not follow
+the encoding rules and probably many other rules.
OPTIONS
-------
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 7f5cd2a..31db699 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -157,6 +157,145 @@ test_expect_success 'with non-trailer lines only' '
test_cmp expected actual
'
+test_expect_success 'multiline field treated as atomic for placement' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: value on
+ Qmultiple lines
+ another: trailer
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: value on
+ Qmultiple lines
+ name: value
+ another: trailer
+ EOF
+ test_config trailer.name.where after &&
+ git interpret-trailers --trailer "name: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for replacement' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: value on
+ Qmultiple lines
+ another: trailer
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ another: trailer
+ name: value
+ EOF
+ test_config trailer.name.ifexists replace &&
+ git interpret-trailers --trailer "name: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for difference check' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ test_config trailer.name.ifexists addIfDifferent &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ Qsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ QQQQQsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ name: first line
+ QQQQQsecond line
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line *DIFFERENT*
+ Qsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ name: first line *DIFFERENT*
+ Qsecond line
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for neighbor check' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ test_config trailer.name.where after &&
+ test_config trailer.name.ifexists addIfDifferentNeighbor &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ Qsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ QQQQQsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ name: first line
+ QQQQQsecond line
+ another: trailer
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'with config setup' '
git config trailer.ack.key "Acked-by: " &&
cat >expected <<-\EOF &&
diff --git a/trailer.c b/trailer.c
index d6dfc7a..ef276e6 100644
--- a/trailer.c
+++ b/trailer.c
@@ -248,7 +248,7 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
if (arg_tok->value && arg_tok->value[0]) {
arg = arg_tok->value;
} else {
- if (in_tok && in_tok->value)
+ if (in_tok)
arg = xstrdup(in_tok->value);
else
arg = xstrdup("");
@@ -622,12 +622,14 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
return 0;
}
-static void add_trailer_item(struct list_head *head, char *tok, char *val)
+static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
+ char *val)
{
struct trailer_item *new = xcalloc(sizeof(*new), 1);
new->token = tok;
new->value = val;
list_add_tail(&new->list, head);
+ return new;
}
static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
@@ -781,6 +783,7 @@ static int process_input_file(FILE *outfile,
int patch_start, trailer_start, trailer_end, i;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
+ struct trailer_item *last = NULL;
/* Get the line count */
while (lines[count])
@@ -798,16 +801,25 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
+ if (last && isspace(lines[i]->buf[0])) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addf(&sb, "%s\n%s", last->value, lines[i]->buf);
+ strbuf_strip_suffix(&sb, "\n");
+ free(last->value);
+ last->value = strbuf_detach(&sb, NULL);
+ continue;
+ }
if (!parse_trailer(&tok, &val, NULL, lines[i]->buf, 0))
- add_trailer_item(head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ last = add_trailer_item(head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL));
else {
strbuf_addbuf(&val, lines[i]);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(head,
NULL,
strbuf_detach(&val, NULL));
+ last = NULL;
}
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 5/6] trailer: allow non-trailers in trailer block
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Currently, interpret-trailers requires all lines of a trailer block to
be trailers (or comments) - if not it would not identify that block as a
trailer block, and thus create its own trailer block, inserting a blank
line. For example:
echo -e "\na: b\nnot trailer" |
git interpret-trailers --trailer "c: d"
would result in:
a: b
not trailer
c: d
Relax the definition of a trailer block to only require 1 trailer, so
that trailers can be directly added to such blocks, resulting in:
a: b
not trailer
c: d
This allows arbitrary lines to be included in trailer blocks, like those
in [1], and still allow interpret-trailers to be used.
This change also makes comments in the trailer block be treated as any
other non-trailer line, preserving them in the output of
interpret-trailers.
[1]
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/e7d316a02f683864a12389f8808570e37fb90aa3
---
Documentation/git-interpret-trailers.txt | 3 +-
t/t7513-interpret-trailers.sh | 35 +++++++++++++++
trailer.c | 77 ++++++++++++++++++++++----------
3 files changed, 90 insertions(+), 25 deletions(-)
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 93d1db6..c480da6 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -48,7 +48,8 @@ with only spaces at the end of the commit message part, one blank line
will be added before the new trailer.
Existing trailers are extracted from the input message by looking for
-a group of one or more lines that contain a colon (by default), where
+a group of one or more lines in which at least one line contains a
+colon (by default), where
the group is preceded by one or more empty (or whitespace-only) lines.
The group must either be at the end of the message or be the last
non-whitespace lines before a line that starts with '---'. Such three
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index aee785c..7f5cd2a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -126,6 +126,37 @@ test_expect_success 'with multiline title in the message' '
test_cmp expected actual
'
+test_expect_success 'with non-trailer lines mixed with trailer lines' '
+ cat >patch <<-\EOF &&
+
+ this: is a trailer
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this: is a trailer
+ this is not a trailer
+ token: value
+ EOF
+ git interpret-trailers --trailer "token: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'with non-trailer lines only' '
+ cat >patch <<-\EOF &&
+
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this is not a trailer
+
+ token: value
+ EOF
+ git interpret-trailers --trailer "token: value" patch >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'with config setup' '
git config trailer.ack.key "Acked-by: " &&
cat >expected <<-\EOF &&
@@ -257,6 +288,8 @@ test_expect_success 'with message that has comments' '
cat >>expected <<-\EOF &&
# comment
+ # other comment
+ # yet another comment
Reviewed-by: Johan
Cc: Peff
# last comment
@@ -286,6 +319,8 @@ test_expect_success 'with message that has an old style conflict block' '
cat >>expected <<-\EOF &&
# comment
+ # other comment
+ # yet another comment
Reviewed-by: Johan
Cc: Peff
# last comment
diff --git a/trailer.c b/trailer.c
index a9ed3f8..d6dfc7a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -27,6 +27,10 @@ static struct conf_info default_conf_info;
struct trailer_item {
struct list_head list;
+ /*
+ * If this is not a trailer line, the line is stored in value
+ * (excluding the terminating newline) and token is NULL.
+ */
char *token;
char *value;
};
@@ -70,9 +74,14 @@ static size_t token_len_without_separator(const char *token, size_t len)
static int same_token(struct trailer_item *a, struct arg_item *b)
{
- size_t a_len = token_len_without_separator(a->token, strlen(a->token));
- size_t b_len = token_len_without_separator(b->token, strlen(b->token));
- size_t min_len = (a_len > b_len) ? b_len : a_len;
+ size_t a_len, b_len, min_len;
+
+ if (!a->token)
+ return 0;
+
+ a_len = token_len_without_separator(a->token, strlen(a->token));
+ b_len = token_len_without_separator(b->token, strlen(b->token));
+ min_len = (a_len > b_len) ? b_len : a_len;
return !strncasecmp(a->token, b->token, min_len);
}
@@ -130,7 +139,14 @@ static char last_non_space_char(const char *s)
static void print_tok_val(FILE *outfile, const char *tok, const char *val)
{
- char c = last_non_space_char(tok);
+ char c;
+
+ if (!tok) {
+ fprintf(outfile, "%s\n", val);
+ return;
+ }
+
+ c = last_non_space_char(tok);
if (!c)
return;
if (strchr(separators, c))
@@ -543,8 +559,16 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
}
+/*
+ * Parse the given trailer into token and value parts.
+ *
+ * If the given trailer does not have a separator (and thus cannot be separated
+ * into token and value parts), it is treated as a token (if parse_as_arg) or
+ * as a non-trailer line (if not parse_as_arg).
+ */
static int parse_trailer(struct strbuf *tok, struct strbuf *val,
- const struct conf_info **conf, const char *trailer)
+ const struct conf_info **conf, const char *trailer,
+ int parse_as_arg)
{
size_t len;
struct strbuf seps = STRBUF_INIT;
@@ -557,11 +581,18 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
len = strcspn(trailer, seps.buf);
strbuf_release(&seps);
if (len == 0) {
- int l = strlen(trailer);
+ int l;
+ if (!parse_as_arg)
+ return -1;
+
+ l = strlen(trailer);
while (l > 0 && isspace(trailer[l - 1]))
l--;
return error(_("empty trailer token in trailer '%.*s'"), l, trailer);
}
+ if (!parse_as_arg && len == strlen(trailer))
+ return -1;
+
if (len < strlen(trailer)) {
strbuf_add(tok, trailer, len);
strbuf_trim(tok);
@@ -631,7 +662,7 @@ static void process_command_line_args(struct list_head *arg_head,
/* Add an arg item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
- if (!parse_trailer(&tok, &val, &conf, tr->string))
+ if (!parse_trailer(&tok, &val, &conf, tr->string, 1))
add_arg_item(arg_head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
@@ -683,7 +714,7 @@ static int find_patch_start(struct strbuf **lines, int count)
*/
static int find_trailer_start(struct strbuf **lines, int count)
{
- int start, end_of_title, only_spaces = 1;
+ int start, end_of_title, only_spaces = 1, trailer_found = 0;
/* The first paragraph is the title and cannot be trailers */
for (start = 0; start < count; start++) {
@@ -699,22 +730,17 @@ static int find_trailer_start(struct strbuf **lines, int count)
* for a line with only spaces before lines with one separator.
*/
for (start = count - 1; start >= end_of_title; start--) {
- if (lines[start]->buf[0] == comment_line_char)
- continue;
if (contains_only_spaces(lines[start]->buf)) {
if (only_spaces)
continue;
- return start + 1;
+ return trailer_found ? start + 1 : count;
}
- if (strcspn(lines[start]->buf, separators) < lines[start]->len) {
- if (only_spaces)
- only_spaces = 0;
- continue;
- }
- return count;
+ only_spaces = 0;
+ if (strcspn(lines[start]->buf, separators) < lines[start]->len)
+ trailer_found = 1;
}
- return only_spaces ? count : 0;
+ return count;
}
/* Get the index of the end of the trailers */
@@ -735,11 +761,8 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)
static int has_blank_line_before(struct strbuf **lines, int start)
{
- for (;start >= 0; start--) {
- if (lines[start]->buf[0] == comment_line_char)
- continue;
+ if (start >= 0)
return contains_only_spaces(lines[start]->buf);
- }
return 0;
}
@@ -775,11 +798,17 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
- if (lines[i]->buf[0] != comment_line_char &&
- !parse_trailer(&tok, &val, NULL, lines[i]->buf))
+ if (!parse_trailer(&tok, &val, NULL, lines[i]->buf, 0))
add_trailer_item(head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL));
+ else {
+ strbuf_addbuf(&val, lines[i]);
+ strbuf_strip_suffix(&val, "\n");
+ add_trailer_item(head,
+ NULL,
+ strbuf_detach(&val, NULL));
+ }
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 4/6] trailer: make args have their own struct
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Improve type safety by making arguments (whether from configuration or
from the command line) have their own "struct arg_item" type, separate
from the "struct trailer_item" type used to represent the trailers in
the buffer being manipulated.
This change also prepares "struct trailer_item" to be further
differentiated from "struct arg_item" in future patches.
---
trailer.c | 135 +++++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 85 insertions(+), 50 deletions(-)
diff --git a/trailer.c b/trailer.c
index 54cc930..a9ed3f8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -29,6 +29,12 @@ struct trailer_item {
struct list_head list;
char *token;
char *value;
+};
+
+struct arg_item {
+ struct list_head list;
+ char *token;
+ char *value;
struct conf_info conf;
};
@@ -62,7 +68,7 @@ static size_t token_len_without_separator(const char *token, size_t len)
return len;
}
-static int same_token(struct trailer_item *a, struct trailer_item *b)
+static int same_token(struct trailer_item *a, struct arg_item *b)
{
size_t a_len = token_len_without_separator(a->token, strlen(a->token));
size_t b_len = token_len_without_separator(b->token, strlen(b->token));
@@ -71,12 +77,12 @@ static int same_token(struct trailer_item *a, struct trailer_item *b)
return !strncasecmp(a->token, b->token, min_len);
}
-static int same_value(struct trailer_item *a, struct trailer_item *b)
+static int same_value(struct trailer_item *a, struct arg_item *b)
{
return !strcasecmp(a->value, b->value);
}
-static int same_trailer(struct trailer_item *a, struct trailer_item *b)
+static int same_trailer(struct trailer_item *a, struct arg_item *b)
{
return same_token(a, b) && same_value(a, b);
}
@@ -98,6 +104,13 @@ static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *
static void free_trailer_item(struct trailer_item *item)
{
+ free(item->token);
+ free(item->value);
+ free(item);
+}
+
+static void free_arg_item(struct arg_item *item)
+{
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
@@ -137,17 +150,29 @@ static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
}
}
+static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
+{
+ struct trailer_item *new = xcalloc(sizeof(*new), 1);
+ new->token = arg_tok->token;
+ new->value = arg_tok->value;
+ arg_tok->token = arg_tok->value = NULL;
+ free_arg_item(arg_tok);
+ return new;
+}
+
static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
- if (after_or_end(arg_tok->conf.where))
- list_add(&arg_tok->list, &on_tok->list);
+ int aoe = after_or_end(arg_tok->conf.where);
+ struct trailer_item *to_add = trailer_from_arg(arg_tok);
+ if (aoe)
+ list_add(&to_add->list, &on_tok->list);
else
- list_add_tail(&arg_tok->list, &on_tok->list);
+ list_add_tail(&to_add->list, &on_tok->list);
}
static int check_if_different(struct trailer_item *in_tok,
- struct trailer_item *arg_tok,
+ struct arg_item *arg_tok,
int check_all,
struct list_head *head)
{
@@ -200,7 +225,7 @@ static char *apply_command(const char *command, const char *arg)
return result;
}
-static void apply_item_command(struct trailer_item *in_tok, struct trailer_item *arg_tok)
+static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
{
if (arg_tok->conf.command) {
const char *arg;
@@ -218,13 +243,13 @@ static void apply_item_command(struct trailer_item *in_tok, struct trailer_item
}
static void apply_arg_if_exists(struct trailer_item *in_tok,
- struct trailer_item *arg_tok,
+ struct arg_item *arg_tok,
struct trailer_item *on_tok,
struct list_head *head)
{
switch (arg_tok->conf.if_exists) {
case EXISTS_DO_NOTHING:
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
case EXISTS_REPLACE:
apply_item_command(in_tok, arg_tok);
@@ -241,39 +266,41 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
if (check_if_different(in_tok, arg_tok, 1, head))
add_arg_to_input_list(on_tok, arg_tok);
else
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
apply_item_command(in_tok, arg_tok);
if (check_if_different(on_tok, arg_tok, 0, head))
add_arg_to_input_list(on_tok, arg_tok);
else
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
}
}
static void apply_arg_if_missing(struct list_head *head,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
enum action_where where;
+ struct trailer_item *to_add;
switch (arg_tok->conf.if_missing) {
case MISSING_DO_NOTHING:
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
case MISSING_ADD:
where = arg_tok->conf.where;
apply_item_command(NULL, arg_tok);
+ to_add = trailer_from_arg(arg_tok);
if (after_or_end(where))
- list_add_tail(&arg_tok->list, head);
+ list_add_tail(&to_add->list, head);
else
- list_add(&arg_tok->list, head);
+ list_add(&to_add->list, head);
}
}
static int find_same_and_apply_arg(struct list_head *head,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
struct list_head *pos;
struct trailer_item *in_tok;
@@ -306,11 +333,11 @@ static void process_trailers_lists(struct list_head *head,
struct list_head *arg_head)
{
struct list_head *pos, *p;
- struct trailer_item *arg_tok;
+ struct arg_item *arg_tok;
list_for_each_safe(pos, p, arg_head) {
int applied = 0;
- arg_tok = list_entry(pos, struct trailer_item, list);
+ arg_tok = list_entry(pos, struct arg_item, list);
list_del(pos);
@@ -375,20 +402,20 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
dst->command = xstrdup(src->command);
}
-static struct trailer_item *get_conf_item(const char *name)
+static struct arg_item *get_conf_item(const char *name)
{
struct list_head *pos;
- struct trailer_item *item;
+ struct arg_item *item;
/* Look up item with same name */
list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct trailer_item, list);
+ item = list_entry(pos, struct arg_item, list);
if (!strcasecmp(item->conf.name, name))
return item;
}
/* Item does not already exists, create it */
- item = xcalloc(sizeof(struct trailer_item), 1);
+ item = xcalloc(sizeof(*item), 1);
duplicate_conf(&item->conf, &default_conf_info);
item->conf.name = xstrdup(name);
@@ -442,7 +469,7 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v
static int git_trailer_config(const char *conf_key, const char *value, void *cb)
{
const char *trailer_item, *variable_name;
- struct trailer_item *item;
+ struct arg_item *item;
struct conf_info *conf;
char *name = NULL;
enum trailer_info_type type;
@@ -500,7 +527,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
return 0;
}
-static const char *token_from_item(struct trailer_item *item, char *tok)
+static const char *token_from_item(struct arg_item *item, char *tok)
{
if (item->conf.key)
return item->conf.key;
@@ -509,7 +536,7 @@ static const char *token_from_item(struct trailer_item *item, char *tok)
return item->conf.name;
}
-static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)
+static int token_matches_item(const char *tok, struct arg_item *item, int tok_len)
{
if (!strncasecmp(tok, item->conf.name, tok_len))
return 1;
@@ -521,7 +548,7 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
{
size_t len;
struct strbuf seps = STRBUF_INIT;
- struct trailer_item *item;
+ struct arg_item *item;
int tok_len;
struct list_head *pos;
@@ -547,12 +574,14 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
/* Lookup if the token matches something in the config */
tok_len = token_len_without_separator(tok->buf, tok->len);
- *conf = &default_conf_info;
+ if (conf)
+ *conf = &default_conf_info;
list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct trailer_item, list);
+ item = list_entry(pos, struct arg_item, list);
if (token_matches_item(tok->buf, item, tok_len)) {
char *tok_buf = strbuf_detach(tok, NULL);
- *conf = &item->conf;
+ if (conf)
+ *conf = &item->conf;
strbuf_addstr(tok, token_from_item(item, tok_buf));
free(tok_buf);
break;
@@ -562,43 +591,51 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
return 0;
}
-static void add_trailer_item(struct list_head *head, char *tok, char *val,
- const struct conf_info *conf)
+static void add_trailer_item(struct list_head *head, char *tok, char *val)
{
struct trailer_item *new = xcalloc(sizeof(*new), 1);
new->token = tok;
new->value = val;
- duplicate_conf(&new->conf, conf);
list_add_tail(&new->list, head);
}
+static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
+ const struct conf_info *conf)
+{
+ struct arg_item *new = xcalloc(sizeof(*new), 1);
+ new->token = tok;
+ new->value = val;
+ duplicate_conf(&new->conf, conf);
+ list_add_tail(&new->list, arg_head);
+}
+
static void process_command_line_args(struct list_head *arg_head,
struct string_list *trailers)
{
struct string_list_item *tr;
- struct trailer_item *item;
+ struct arg_item *item;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
const struct conf_info *conf;
struct list_head *pos;
- /* Add a trailer item for each configured trailer with a command */
+ /* Add an arg item for each configured trailer with a command */
list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct trailer_item, list);
+ item = list_entry(pos, struct arg_item, list);
if (item->conf.command)
- add_trailer_item(arg_head,
- xstrdup(token_from_item(item, NULL)),
- xstrdup(""),
- &item->conf);
+ add_arg_item(arg_head,
+ xstrdup(token_from_item(item, NULL)),
+ xstrdup(""),
+ &item->conf);
}
- /* Add a trailer item for each trailer on the command line */
+ /* Add an arg item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
if (!parse_trailer(&tok, &val, &conf, tr->string))
- add_trailer_item(arg_head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf);
+ add_arg_item(arg_head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
}
@@ -721,7 +758,6 @@ static int process_input_file(FILE *outfile,
int patch_start, trailer_start, trailer_end, i;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
- const struct conf_info *conf;
/* Get the line count */
while (lines[count])
@@ -740,11 +776,10 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
if (lines[i]->buf[0] != comment_line_char &&
- !parse_trailer(&tok, &val, &conf, lines[i]->buf))
+ !parse_trailer(&tok, &val, NULL, lines[i]->buf))
add_trailer_item(head,
strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf);
+ strbuf_detach(&val, NULL));
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ 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