# [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree 20 messages from 2010-12-25 to 2011-02-03. Participants: Zapped, Jens Lehmann, Johannes Schindelin, Junio C Hamano, Алексей Крезов, Алексей Шумкин, Casey Dahlin, Thomas Rast, Eric Wong, Alexey Shumkin. Thread: https://gitlist.dev/t/26135 ## Zapped, 2010-12-25 01:20 Subject: [PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal Message-ID: <1293240049-7744-1-git-send-email-zapped@mail.ru> URL: https://gitlist.dev/e/1293240049-7744-1-git-send-email-zapped%40mail.ru ``` Signed-off-by: Zapped --- userdiff.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/userdiff.c b/userdiff.c index f9e05b5..259a382 100644 --- a/userdiff.c +++ b/userdiff.c @@ -52,7 +52,7 @@ PATTERNS("objc", "|[-+*/<>%&^|=!]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\||::|->" "|[^[:space:]]|[\x80-\xff]+"), PATTERNS("pascal", - "^((procedure|function|constructor|destructor|interface|" + "^(((class[ \t]+)?(procedure|function)|constructor|destructor|interface|" "implementation|initialization|finalization)[ \t]*.*)$" "\n" "^(.*=[ \t]*(class|record).*)$", -- 1.7.3.4.3.g3f811 ``` ## Zapped, 2010-12-25 01:20 Subject: [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree Message-ID: <1293240049-7744-2-git-send-email-zapped@mail.ru> URL: https://gitlist.dev/e/1293240049-7744-2-git-send-email-zapped%40mail.ru In-Reply-To: <1293240049-7744-1-git-send-email-zapped@mail.ru> ``` git-svn dcommit/set-tree fails when svn.pathnameencoding is set for native OS encoding (e.g. cp1251 for Windows) though git-svn fetch/clone works well Signed-off-by: Zapped --- git-svn.perl | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index 757de82..399bf4c 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -4451,6 +4451,7 @@ sub new { $self->{path_prefix} = length $self->{svn_path} ? "$self->{svn_path}/" : ''; $self->{config} = $opts->{config}; + $self->{pathnameencoding} = Git::config('svn.pathnameencoding'); return $self; } -- 1.7.3.4.3.g3f811 ``` ## Zapped, 2010-12-25 01:20 Subject: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: <1293240049-7744-3-git-send-email-zapped@mail.ru> URL: https://gitlist.dev/e/1293240049-7744-3-git-send-email-zapped%40mail.ru In-Reply-To: <1293240049-7744-1-git-send-email-zapped@mail.ru> ``` Signed-off-by: Zapped --- contrib/completion/git-completion.bash | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index d3037fc..50fc385 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -280,7 +280,8 @@ __git_ps1 () elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then - git diff --no-ext-diff --quiet --exit-code || w="*" + is=$(git config diff.ignoreSubmodules) + git diff --no-ext-diff --quiet --exit-code --ignore-submodules=$is || w="*" if git rev-parse --quiet --verify HEAD >/dev/null; then git diff-index --cached --quiet HEAD -- || i="+" else -- 1.7.3.4.3.g3f811 ``` ## Jens Lehmann, 2010-12-25 12:33 Subject: Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: <4D15E48A.9050805@web.de> URL: https://gitlist.dev/e/4D15E48A.9050805%40web.de In-Reply-To: <1293240049-7744-3-git-send-email-zapped@mail.ru> ``` Am 25.12.2010 02:20, schrieb Zapped: > Signed-off-by: Zapped > --- > contrib/completion/git-completion.bash | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index d3037fc..50fc385 100755 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -280,7 +280,8 @@ __git_ps1 () > elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then > if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then > if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then > - git diff --no-ext-diff --quiet --exit-code || w="*" > + is=$(git config diff.ignoreSubmodules) > + git diff --no-ext-diff --quiet --exit-code --ignore-submodules=$is || w="*" > if git rev-parse --quiet --verify HEAD >/dev/null; then > git diff-index --cached --quiet HEAD -- || i="+" > else Thanks for resubmitting this as an inline patch for review (although it would have been easier for me if the commit message would have described the problem you tried to fix a bit more in detail ;-). After testing this patch it looks like it has a few issues: 1) it will break any per-submodule configuration done via the 'submodule..ignore' setting in .git/config or .gitmodules, as using the --ignore-submodules option overrides those while only setting 'diff.ignoreSubmodules' should not do that. 2) If diff.ignoreSubmodules is unset it leads to an error every time the prompt is displayed: 'fatal: bad --ignore-submodules argument:' 3) And for me it didn't change the behavior at all: - The '*' in the prompt vanishes as I set diff.ignoreSubmodules as expected with or without your patch. Am I missing something here? - The real problem here is that the '+' never goes away even when 'diff.ignoreSubmodules' is set to 'all'. This is due to the fact that 'diff.ignoreSubmodules' is only honored by "git diff", but not by "git diff-index". So the real issue here seems to be the "git diff-index" call, which doesn't honor the 'diff.ignoreSubmodules' setting. In commit 37aea37 Dscho (CCed) introduced this configuration setting while explicitly stating that it only affects porcelain. As the other config options always influence porcelain and plumbing, it looks like we would want to have this option honored by plumbing too, no? So are there any reasons for the plumbing diff commands not to honor the diff.ignoreSubmodules setting? ``` ## Johannes Schindelin, 2010-12-25 13:08 Subject: Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.1012251407230.1822%40bonsai2 In-Reply-To: <4D15E48A.9050805@web.de> ``` Hi, On Sat, 25 Dec 2010, Jens Lehmann wrote: > In commit 37aea37 Dscho (CCed) introduced this configuration setting > while explicitly stating that it only affects porcelain. As the other > config options always influence porcelain and plumbing, it looks like we > would want to have this option honored by plumbing too, no? IIRC Junio was real happy that I did not honor plumbing. Ciao, Dscho ``` ## Junio C Hamano, 2010-12-26 19:14 Subject: Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: <7vd3ooz6qd.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd3ooz6qd.fsf%40alter.siamese.dyndns.org In-Reply-To: <4D15E48A.9050805@web.de> ``` Jens Lehmann writes: > So are there any reasons for the plumbing diff commands not to honor > the diff.ignoreSubmodules setting? There are two kinds of users of the plumbing. One class of plumbing users is scripts that is about automation and mechanization that want to control what they do precisely (think cron jobs) without getting affected by the user preference stored in the repository configuration. This class could either: (1) state what they want explicitly from the command line; or (2) rely on built-in defaults not changing underneath them. The behaviour of diff recursively inspecting submodule dirtiness has an unfortunate history, in that the behaviour changed over time, and in each step when we made a change, we thought we were making an unquestionable improvement. Originally we only said "submodule HEAD is different from what we have in the index/superproject HEAD". Later we added different kind of dirtiness like untracked files or modified contents in submodules, decided perhaps mistakenly that majority of users do want to see them as dirtiness and made that the default and allowed them to be ignored by an explicit request. At that point, in order not to break existing scripts (mostly of the "mechanization" class, written back when there was no such extra dirtyness hence with no "explicit refusal" route available to them without rewriting), hence "no configuration should affect plumbing randomly" policy. On the other hand, you may write user facing Porcelain in scripts and run plumbing from there. This class of plumbing users could either: (1) inspect the config itself, interpret the customization and pass an explicit command line flag; or (2) allow the plumbing honor the end user configuration stored in the repository or user configuration files. It is argurably more convenient for these users if the plumbing blindly honored the configurations, as it would have allowed the latter implementation. That way, we can be more lazy when writing our scripts, and ignore having to worry about new kinds of customization added to underlying git after a script is written---but new kinds of customization may break your script's expectation of what will and what will not be made customizable, and you would end up giving an explicit "do not use that feature" in some cases, so the being able to be lazy is not necessarily always a win. Things may have been a bit different if the original feature change to inspect submodules deeper, command line flags to control that behaviour and configuration to default the flags came at the same time, but unfortunately they happend over time. I think we have been slowly getting better at this, but in the case of this particular feature, the original introduction of --ignore-submodules was in May 2008, deeper submodule inspection and the richer --ignore-submodules= option came much later in June 2010, and the configuration was invented later in August 2010, which would mean that allowing the plumbing to honor configuration would have broken scripts written in the 2 years and 3 months period. And no, this does not call for a blanket "do / do not honor configuration" option to plumbing commands. A more selective "do / do not honor these configuration variables" option might be an option, though. By the way, could we please have a real sign-off, not with a one with a pseudonym, given to the series? ``` ## Алексей Крезов, 2010-12-26 22:25 Subject: Re[2]: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: <1824011293.20101227012557@mail.ru> URL: https://gitlist.dev/e/1824011293.20101227012557%40mail.ru In-Reply-To: <4D15E48A.9050805@web.de> ``` Hello! I`m sorry, but I`m newbie in making and distributing of public patches. So, don't beat me much )) JL> it would have been easier for me if the commit message would have JL> described the problem you tried to fix a bit more in detail ;-). My problem was in the following. I use Git on Windows XP under Cygwin. So its perfomance is slower than on *nix, I guess. My project has 40 submodules (external libs) and some of them could have untracked files (for some reasons). So when I run any command in Bash after its execution Bash "thought" for 2-3 seconds. That was annoying. I do not remember the version of Git I used at that moment but I remember it was an update from 1.6.x to early 1.7.x. So I decided to roll back to 1.6.x )) Then there was some updates of Git. But after updating the problem still happened. When I tried to discover the reason of such a behaviour I found that Git got status for all submodules including untracked content and so marked them with * Then I read manual and found diff.ignoreSubmodules and tried to set up for each submodule in a .gitmodules but nothing changed (as it seemed to me at that time). So I've found the easiest way to solve my problem - this patch ) Maybe after this patch there was some changes in Git solved this problem but I did not investigate it, sorry. JL> 2) If diff.ignoreSubmodules is unset it leads to an error JL> every time the prompt is displayed: JL> 'fatal: bad --ignore-submodules argument:' Yeah, you're right JL> Am 25.12.2010 02:20, schrieb Zapped: >> Signed-off-by: Zapped >> --- >> contrib/completion/git-completion.bash | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash >> index d3037fc..50fc385 100755 >> --- a/contrib/completion/git-completion.bash >> +++ b/contrib/completion/git-completion.bash >> @@ -280,7 +280,8 @@ __git_ps1 () >> elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then >> if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then >> if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then >> - git diff --no-ext-diff --quiet --exit-code || w="*" >> + is=$(git config diff.ignoreSubmodules) >> + git diff --no-ext-diff --quiet --exit-code --ignore-submodules=$is || w="*" >> if git rev-parse --quiet --verify HEAD >/dev/null; then >> git diff-index --cached --quiet HEAD -- || i="+" >> else JL> Thanks for resubmitting this as an inline patch for review (although JL> it would have been easier for me if the commit message would have JL> described the problem you tried to fix a bit more in detail ;-). JL> After testing this patch it looks like it has a few issues: JL> 1) it will break any per-submodule configuration done via JL> the 'submodule..ignore' setting in .git/config or JL> .gitmodules, as using the --ignore-submodules option JL> overrides those while only setting 'diff.ignoreSubmodules' JL> should not do that. JL> 2) If diff.ignoreSubmodules is unset it leads to an error JL> every time the prompt is displayed: JL> 'fatal: bad --ignore-submodules argument:' JL> 3) And for me it didn't change the behavior at all: JL> - The '*' in the prompt vanishes as I set diff.ignoreSubmodules JL> as expected with or without your patch. JL> Am I missing something here? JL> - The real problem here is that the '+' never goes away even JL> when 'diff.ignoreSubmodules' is set to 'all'. This is due JL> to the fact that 'diff.ignoreSubmodules' is only honored by JL> "git diff", but not by "git diff-index". JL> So the real issue here seems to be the "git diff-index" call, which JL> doesn't honor the 'diff.ignoreSubmodules' setting. In commit 37aea37 JL> Dscho (CCed) introduced this configuration setting while explicitly JL> stating that it only affects porcelain. As the other config options JL> always influence porcelain and plumbing, it looks like we would want JL> to have this option honored by plumbing too, no? JL> So are there any reasons for the plumbing diff commands not to honor JL> the diff.ignoreSubmodules setting? ``` ## Алексей Шумкин, 2010-12-26 22:42 Subject: Re[2]: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: <255471532.20101227014203@mail.ru> URL: https://gitlist.dev/e/255471532.20101227014203%40mail.ru In-Reply-To: <7vd3ooz6qd.fsf@alter.siamese.dyndns.org> ``` JCH> By the way, could we please have a real sign-off, not with a one with a JCH> pseudonym, given to the series? Oh, I`m sorry. Of course. Should I resend patches with real sign-off, at least first two ones? ``` ## Jens Lehmann, 2010-12-27 11:14 Subject: Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: <4D187511.3090104@web.de> URL: https://gitlist.dev/e/4D187511.3090104%40web.de In-Reply-To: <7vd3ooz6qd.fsf@alter.siamese.dyndns.org> ``` Am 26.12.2010 20:14, schrieb Junio C Hamano: > Jens Lehmann writes: > >> So are there any reasons for the plumbing diff commands not to honor >> the diff.ignoreSubmodules setting? Thank you for explaining those reasons in detail. > One class of plumbing users is scripts that is about automation and > mechanization that want to control what they do precisely (think cron > jobs) without getting affected by the user preference stored in the > repository configuration. This class could either: > > (1) state what they want explicitly from the command line; or > (2) rely on built-in defaults not changing underneath them. > > The behaviour of diff recursively inspecting submodule dirtiness has an > unfortunate history, in that the behaviour changed over time, and in each > step when we made a change, we thought we were making an unquestionable > improvement. Originally we only said "submodule HEAD is different from > what we have in the index/superproject HEAD". Later we added different > kind of dirtiness like untracked files or modified contents in submodules, > decided perhaps mistakenly that majority of users do want to see them as > dirtiness and made that the default and allowed them to be ignored by an > explicit request. At that point, in order not to break existing scripts > (mostly of the "mechanization" class, written back when there was no such > extra dirtyness hence with no "explicit refusal" route available to them > without rewriting), hence "no configuration should affect plumbing > randomly" policy. Good point. But unfortunately the diff plumbing commands are affected by the "submodule..ignore" ignore settings I introduced in August in aee9c7d6 and 302ad7a9. Maybe we should revert the part of these patches that changed the plumbing commands? > On the other hand, you may write user facing Porcelain in scripts and run > plumbing from there. This class of plumbing users could either: > > (1) inspect the config itself, interpret the customization and pass > an explicit command line flag; or > > (2) allow the plumbing honor the end user configuration stored in the > repository or user configuration files. > > It is argurably more convenient for these users if the plumbing blindly > honored the configurations, as it would have allowed the latter > implementation. That way, we can be more lazy when writing our scripts, > and ignore having to worry about new kinds of customization added to > underlying git after a script is written---but new kinds of customization > may break your script's expectation of what will and what will not be made > customizable, and you would end up giving an explicit "do not use that > feature" in some cases, so the being able to be lazy is not necessarily > always a win. Agreed. > Things may have been a bit different if the original feature change to > inspect submodules deeper, command line flags to control that behaviour > and configuration to default the flags came at the same time, but > unfortunately they happend over time. I think we have been slowly getting > better at this, but in the case of this particular feature, the original > introduction of --ignore-submodules was in May 2008, deeper submodule > inspection and the richer --ignore-submodules= option came much > later in June 2010, and the configuration was invented later in August > 2010, which would mean that allowing the plumbing to honor configuration > would have broken scripts written in the 2 years and 3 months period. > > And no, this does not call for a blanket "do / do not honor configuration" > option to plumbing commands. A more selective "do / do not honor these > configuration variables" option might be an option, though. What about a new "--ignore-submodules=config" option to tell the plumbing that it should honor the config? And it looks like the PS1 problem that started this discussion is a valid example for mixed usage of porcelain and plumbing commands. In a first attempt to fix the problem by using "git diff --cached" instead of "git diff-index --cached" I noticed that those two commands give different results when new submodules were created and had been added to the index. "git diff --cached" ignores them while "git diff-index --cached" shows them. Anything I am missing here? ``` ## Johannes Schindelin, 2010-12-27 22:06 Subject: Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.1012272256560.1794%40bonsai2 In-Reply-To: <4D187511.3090104@web.de> ``` Hi Jens, On Mon, 27 Dec 2010, Jens Lehmann wrote: > [...] > > And it looks like the PS1 problem that started this discussion is a > valid example for mixed usage of porcelain and plumbing commands. The distinction between porcelain and plumbing commands is not as clear-cut as some people would like it to be (just call "git grep 'git log'" in a git.git checkout). IMHO the reason is that a distinction between porcelain and plumbing makes sense in the world of sanitary engineering, but not necessarily in the world of software (a distinction between assembler vs source code, or GUI vs library makes sense, but not between "programs to be called by humans" and "programs to be called by other programs"). Note: I do not think that the "plumbing" concept was not well-intended, but I doubt that the concept holds up in the face of reality. I fear, though, that we cannot simply abolish the notion "plumbing vs porcelain" from git.git... Ciao, Dscho ``` ## Casey Dahlin, 2010-12-27 22:43 Subject: Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: <20101227224357.GA9947@foucault.redhat.com> URL: https://gitlist.dev/e/20101227224357.GA9947%40foucault.redhat.com In-Reply-To: ``` On Mon, Dec 27, 2010 at 11:06:38PM +0100, Johannes Schindelin wrote: > Note: I do not think that the "plumbing" concept was not well-intended, > but I doubt that the concept holds up in the face of reality. > I've always felt that plumbing commands existed to expose the C portion of git to the bash portion of git. As the latter shrinks plumbing commands make less sense (and offering those commands as a library makes more sense, which is unfortunately close to impossible in the current git source). --CJD ``` ## Алексей Шумкин, 2010-12-28 12:14 Subject: Re[2]: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable Message-ID: <1393005584.20101228151409@mail.ru> URL: https://gitlist.dev/e/1393005584.20101228151409%40mail.ru In-Reply-To: <4D15E48A.9050805@web.de> ``` Hi, all. Well, I rolled back my so-called patch which rose up such a discussion, and what I did see? Annoying behaviuor is vanished and perfomance of $PS1 is acceptable (as fast as with the "patch"). I guess, as I said, something changed in Git since I discovered and solved the problem such a weird way. So, it seems to me, this patch might be considered unnecessary? ``` ## Thomas Rast, 2011-01-04 17:13 Subject: Re: [PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal Message-ID: <201101041813.45053.trast@student.ethz.ch> URL: https://gitlist.dev/e/201101041813.45053.trast%40student.ethz.ch In-Reply-To: <1293240049-7744-1-git-send-email-zapped@mail.ru> ``` Zapped wrote: > Signed-off-by: Zapped As Junio already said, please provide a real name for the sign-off. But I also found the commit message and content confusing, probably because I haven't programmed Pascal for 15 years. You said Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal > PATTERNS("pascal", > - "^((procedure|function|constructor|destructor|interface|" > + "^(((class[ \t]+)?(procedure|function)|constructor|destructor|interface|" > "implementation|initialization|finalization)[ \t]*.*)$" > "\n" > "^(.*=[ \t]*(class|record).*)$", But the last line very conspicuously already mentions 'class', so why does it fail? I had to look up a bit of Pascal syntax. Google helped with http://www.freepascal.org/docs-html/ref/ref.html which answers this. Also, as stated in SubmittingPatches, we generally word the messages as stating the behaviour of the changed version in the present tense. So a better commit message would be userdiff: match Pascal class methods Class declarations were already covered by the second pattern, but class methods have the 'class' keyword in front too. Account for it. Signed-off-by: Алексей Крезов Ok, now I feel silly for only having a two-liner despite my complaints. That being said, I have now verified that the patch is good, and, you can include my Acked-by: Thomas Rast in a reroll if you fix the above. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Thomas Rast, 2011-01-04 17:18 Subject: Re: [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree Message-ID: <201101041818.09365.trast@student.ethz.ch> URL: https://gitlist.dev/e/201101041818.09365.trast%40student.ethz.ch In-Reply-To: <1293240049-7744-2-git-send-email-zapped@mail.ru> ``` Zapped wrote: > git-svn dcommit/set-tree fails when svn.pathnameencoding is set for native OS encoding (e.g. cp1251 for Windows) though git-svn fetch/clone works well I'll let Eric judge whether loading the encoding here is the right fix, but here too the commit message states only what is broken, not why you fixed it that way. Can you elaborate a bit? Also, this should be easy to cover with a test case, can you make one? > git-svn.perl | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/git-svn.perl b/git-svn.perl > index 757de82..399bf4c 100755 > --- a/git-svn.perl > +++ b/git-svn.perl > @@ -4451,6 +4451,7 @@ sub new { > $self->{path_prefix} = length $self->{svn_path} ? > "$self->{svn_path}/" : ''; > $self->{config} = $opts->{config}; > + $self->{pathnameencoding} = Git::config('svn.pathnameencoding'); > return $self; > } > > -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Eric Wong, 2011-01-04 23:20 Subject: Re: [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree Message-ID: <20110104232029.GA15889@dcvr.yhbt.net> URL: https://gitlist.dev/e/20110104232029.GA15889%40dcvr.yhbt.net In-Reply-To: <201101041818.09365.trast@student.ethz.ch> ``` Thomas Rast wrote: > Zapped wrote: > > git-svn dcommit/set-tree fails when svn.pathnameencoding is set for native OS encoding (e.g. cp1251 for Windows) though git-svn fetch/clone works well > > I'll let Eric judge whether loading the encoding here is the right > fix, but here too the commit message states only what is broken, not > why you fixed it that way. Can you elaborate a bit? > > Also, this should be easy to cover with a test case, can you make one? I second Thomas's requests. I'm also a bit disappointed the original option is missing tests, especially since not many people are likely to use it. -- Eric Wong ``` ## Алексей Шумкин, 2011-01-05 11:44 Subject: Re[2]: [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree Message-ID: <707440142.20110105144417@mail.ru> URL: https://gitlist.dev/e/707440142.20110105144417%40mail.ru In-Reply-To: <201101041818.09365.trast@student.ethz.ch> ``` As I already said I'm newbie in submitting/distributing patches but I'll try :) Tuesday, January 4, 2011, 8:18:09 PM, you wrote: TR> Zapped wrote: >> git-svn dcommit/set-tree fails when svn.pathnameencoding is set for native OS encoding (e.g. cp1251 for Windows) though git-svn fetch/clone works well TR> I'll let Eric judge whether loading the encoding here is the right TR> fix, but here too the commit message states only what is broken, not TR> why you fixed it that way. Can you elaborate a bit? TR> Also, this should be easy to cover with a test case, can you make one? >> git-svn.perl | 1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> diff --git a/git-svn.perl b/git-svn.perl >> index 757de82..399bf4c 100755 >> --- a/git-svn.perl >> +++ b/git-svn.perl >> @@ -4451,6 +4451,7 @@ sub new { >> $self->{path_prefix} = length $self->{svn_path} ? >> "$self->{svn_path}/" : ''; >> $self->{config} = $opts->{config}; >> + $self->{pathnameencoding} = Git::config('svn.pathnameencoding'); >> return $self; >> } >> >> -- С уважением, Алексей Шумкин mailto:zapped@mail.ru ``` ## Алексей Шумкин, 2011-01-05 11:53 Subject: Re[2]: [PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal Message-ID: <4510264083.20110105145302@mail.ru> URL: https://gitlist.dev/e/4510264083.20110105145302%40mail.ru In-Reply-To: <201101041813.45053.trast@student.ethz.ch> ``` Hello, Thomas TR> But the last line very conspicuously already mentions 'class', so why TR> does it fail? Yes, As you already discovered that last line match for class/record definition but not for class methods. I did as you said I changed commit message (also included "Acked-by:"). So should I re-submit patch to the maillist as a new one or as an answer to this thread? TR> Zapped wrote: >> Signed-off-by: Zapped TR> As Junio already said, please provide a real name for the sign-off. TR> But I also found the commit message and content confusing, probably TR> because I haven't programmed Pascal for 15 years. TR> You said TR> Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal >> PATTERNS("pascal", >> - "^((procedure|function|constructor|destructor|interface|" >> + "^(((class[ \t]+)?(procedure|function)|constructor|destructor|interface|" >> "implementation|initialization|finalization)[ \t]*.*)$" >> "\n" >> "^(.*=[ \t]*(class|record).*)$", TR> But the last line very conspicuously already mentions 'class', so why TR> does it fail? TR> I had to look up a bit of Pascal syntax. Google helped with TR> http://www.freepascal.org/docs-html/ref/ref.html TR> which answers this. Also, as stated in SubmittingPatches, we TR> generally word the messages as stating the behaviour of the changed TR> version in the present tense. So a better commit message would be TR> userdiff: match Pascal class methods TR> Class declarations were already covered by the second pattern, but TR> class methods have the 'class' keyword in front too. Account for TR> it. TR> Signed-off-by: Алексей Крезов TR> Ok, now I feel silly for only having a two-liner despite my TR> complaints. TR> That being said, I have now verified that the patch is good, and, you TR> can include my TR> Acked-by: Thomas Rast TR> in a reroll if you fix the above. ``` ## Thomas Rast, 2011-01-05 14:23 Subject: Re: [PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal Message-ID: <201101051523.55749.trast@student.ethz.ch> URL: https://gitlist.dev/e/201101051523.55749.trast%40student.ethz.ch In-Reply-To: <4510264083.20110105145302@mail.ru> ``` Алексей Шумкин wrote: > I did as you said I changed commit message (also included > "Acked-by:"). So should I re-submit patch to the maillist as a new one > or as an answer to this thread? It doesn't matter that much; most people resend as a reply to some email in the thread. Just don't distribute the reroll all across the thread :-) What you can do now that you have an Ack, or if you feel otherwise confident that this patch is ready for inclusion, is Cc it to Junio. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Thomas Rast, 2011-01-05 14:31 Subject: Re: [PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal Message-ID: <201101051531.49142.trast@student.ethz.ch> URL: https://gitlist.dev/e/201101051531.49142.trast%40student.ethz.ch In-Reply-To: <201101051523.55749.trast@student.ethz.ch> ``` Thomas Rast wrote: > Алексей Шумкин wrote: > > I did as you said I changed commit message (also included > > "Acked-by:"). So should I re-submit patch to the maillist as a new one > > or as an answer to this thread? > > It doesn't matter that much; most people resend as a reply to some > email in the thread. Just don't distribute the reroll all across the > thread :-) BTW speaking of series (and sorry for not pointing this out earlier): Since these patches are not related to each other at all, it's better if you treat them as separate, not as a series. Making a series is sort of an "all or nothing" hint, meaning that if something holds up one patch in the series, Junio will treat the whole series as stalled, etc. So probably it's better if you resend the acked patch alone, to show that it can stand on its own. Then later repeat the same for the other two patches when you have them ready. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Alexey Shumkin, 2011-02-03 20:28 Subject: Re: [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree Message-ID: URL: https://gitlist.dev/e/loom.20110203T212349-287%40post.gmane.org In-Reply-To: <20110104232029.GA15889@dcvr.yhbt.net> ``` Eric Wong yhbt.net> writes: > > Thomas Rast student.ethz.ch> wrote: > > Zapped wrote: > > > git-svn dcommit/set-tree fails when svn.pathnameencoding is set for native OS encoding (e.g. cp1251 > for Windows) though git-svn fetch/clone works well > > > > I'll let Eric judge whether loading the encoding here is the right > > fix, but here too the commit message states only what is broken, not > > why you fixed it that way. Can you elaborate a bit? > > > > Also, this should be easy to cover with a test case, can you make one? > > I second Thomas's requests. I'm also a bit disappointed the original > option is missing tests, especially since not many people are likely to > use it. > Yes, I remember that, sorry, but I just have no enough time to get into git tests and make my own ones. ```