* Re: Uninitialized submodules as symlinks
From: Heiko Voigt @ 2016-10-13 16:10 UTC (permalink / raw)
To: David Turner; +Cc: git@vger.kernel.org
In-Reply-To: <fd5bcf57f92944c0b7f6f2f8342c342c@exmbdft7.ad.twosigma.com>
On Fri, Oct 07, 2016 at 06:17:05PM +0000, David Turner wrote:
> Presently, uninitialized submodules are materialized in the working
> tree as empty directories. We would like to consider having them be
> symlinks. Specifically, we'd like them to be symlinks into a FUSE
> filesystem which retrieves files on demand.
How about portability? This feature would only work on Unix like
operating systems. You have to be careful to not break Windows since
they do not have symlinks.
Cheers Heiko
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-13 16:05 UTC (permalink / raw)
To: Kevin Daudt
Cc: Jeff King, Junio C Hamano, Larry Finger, Mathieu Lienard--Mayor,
Remi Lespinet, git
In-Reply-To: <20161013153317.GA1698@ikke.info>
Kevin Daudt <me@ikke.info> writes:
> On Wed, Oct 12, 2016 at 07:13:22PM -0400, Jeff King wrote:
>
>> I think the answer is pretty clearly no. It's just that historically we
>> have auto-munged it into something useful. I think the viable options
>> are basically:
>>
>> 1. Tell people not to do that, and to do something RFC compliant like
>> "Stable [4.8+]" <stable@vger.kernel.org>. This is a little funny
>> for git because we otherwise do not require things like
>> rfc-compliant quoting for our name/email pairs. But it Just Works
>> without anybody having to write extra code, or worry about corner
>> cases in parsing.
>>
>> 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.
>
> Comments, surrounded by parenthesis are allowed after the ">" according
> to the RFC, just plain dropping everything comming after that would
> break that support.
Our in-house parser does consider (...) comments, and my patch does not
change how they are handled: they are still kept after the address part.
However, another piece of code does strip everything behind ">":
sub sanitize_address {
...
# remove garbage after email address
$recipient =~ s/(.*>).*$/$1/;
introduced in 831a488b76e0 (git-send-email: remove garbage after email
address, 2012-11-22).
IMHO, it's OK to continue doing this: removing comments from To: and Cc:
is not really a problem (and I think we've seen nobody complain about it
since 2012). But after my patch, these two lines can probably safely be
removed, as there can no longer be "garbage" after the email, only
comments.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: 2 directories same spelling one directory is camel cased
From: Torsten Bögershausen @ 2016-10-13 16:03 UTC (permalink / raw)
To: git
In-Reply-To: <e0a3eb0590552c98638d486db8ad4a12@davidwbrown.name>
On 12.10.16 18:05, David Brown wrote:
> Howdy git gurus,
>
> I have the dubious distinction of working with a remote repo (master) that has a class loader run-time error when cloned, built and executed.
>
> The reason for the runtime issue is a directory hierarchical path has to directories (folders) with the same name spelling but one of the directories is camel-cased. The package names are the same.
>
> The compiler doesn't care but the run-time class loader has an issue with the 2 'same like named' classes.
>
> How to remove the offending directories and files at the locally cloned repo but not push 'deleted' directories and files back to origin/master the remote repo?
>
> Please advise.
>
> Regards.
This email did not resolve from here: David Brown <david@davidwbrown.name>
It is somewhat unclear, which issue the class loader has.
What does
git ls-files | grep -i "sameNameBitDifferent"
say ?
What do you mean with
"How to remove the offending directories" ?
Just run
rm -rf "offending directories" ?
Or, may be
git mv dir1 NewDir1
If you don't want to push, you don't have to, or what do I miss ?
^ permalink raw reply
* Re: git branches & merge
From: Kevin Daudt @ 2016-10-13 15:49 UTC (permalink / raw)
To: Anatoly Borodin; +Cc: webmaster, git
In-Reply-To: <CACNzp2nG1KfFuiSH1UUGVSz1T39PjzfDgCp6vqOVe7-Lph2yNw@mail.gmail.com>
On Wed, Oct 12, 2016 at 04:43:07PM +0200, Anatoly Borodin wrote:
> Hi,
>
>
> the IP will not be overwritten, you'll still have the new IP in
> master. Nothing to worry about :)
>
>
To expand on that, git does a so called 3-way merge. This means git will
look for a common base commit, and compare changes from both sides to
see which side actually made a change.
In your case, the base and the release branch both should show the old
ip, and the master side would show the new IP. This tells git that
master has changed, and not the release branch, and takes the master
side of the change, resulting the new IP to show up.
Hope this helps, Kevin.
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Kevin Daudt @ 2016-10-13 15:33 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Matthieu Moy, Larry Finger,
Mathieu Lienard--Mayor, Remi Lespinet, git
In-Reply-To: <20161012231321.ar2csdmvhdya2q7r@sigill.intra.peff.net>
On Wed, Oct 12, 2016 at 07:13:22PM -0400, Jeff King wrote:
> On Wed, Oct 12, 2016 at 01:53:52PM -0700, Junio C Hamano wrote:
>
> > Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> >
> > >>> If it's not in the body of the message, then where is it?
> > >>
> > >> This point is clarified in the thread
> > >> http://marc.info/?l=linux-wireless&m=147625930203434&w=2, which is
> > >> with my upstream maintainer.
> > >
> > > Which explicitly states that the syntax is not [$number], but # $number,
> > > right?
> >
> > But I do not think that works, either. Let's step back.
> >
> > 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 the answer is pretty clearly no. It's just that historically we
> have auto-munged it into something useful. I think the viable options
> are basically:
>
> 1. Tell people not to do that, and to do something RFC compliant like
> "Stable [4.8+]" <stable@vger.kernel.org>. This is a little funny
> for git because we otherwise do not require things like
> rfc-compliant quoting for our name/email pairs. But it Just Works
> without anybody having to write extra code, or worry about corner
> cases in parsing.
>
> 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.
Comments, surrounded by parenthesis are allowed after the ">" according
to the RFC, just plain dropping everything comming after that would
break that support.
^ permalink raw reply
* Re: [PATCH v2 1/3] serialize collection of changed submodules
From: Heiko Voigt @ 2016-10-13 15:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqq1szl5vvf.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 10:18:28AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
> > Which seems quite extensively long for a static function so how about
> > we shorten it a bit and add a comment:
> >
> > /* lookup or create commit object list for submodule */
> > get_commit_objects_for_submodule_path(...
>
> Or you can even lose "get_" and "path", I guess. You are not even
> "getting" commits but the array that holds them, so the caller can
> use it to "get" one of them or it can even use it to "put" a new
> one, no? "get-commit-objects" is a misnomer in that sense. Either
> one of
>
> get_submodule_commits_array()
> submodule_commits()
>
> perhaps? I dunno.
I like the last one. Will use 'submodule_commits()'.
Cheers Heiko
^ permalink raw reply
* 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
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