[PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal

[PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal

[Zapped]

Signed-off-by: Zapped <zapped@mail.ru>
---
 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

[PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree

[Zapped]

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 <zapped@mail.ru>
---
 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

Re: [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree

[Thomas Rast]

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

Re: [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree

[Eric Wong]

Thomas Rast <trast@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.

-- 
Eric Wong

Re: [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree

[Alexey Shumkin]

Eric Wong <normalperson <at> yhbt.net> writes:

> 
> Thomas Rast <trast <at> 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.

Re[2]: [PATCH 2/3] Fixes bug: git-svn: svn.pathnameencoding is not respected with dcommit/set-tree

[Алексей Шумкин]

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

[PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Zapped]

Signed-off-by: Zapped <zapped@mail.ru>
---
 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

Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Jens Lehmann]

Am 25.12.2010 02:20, schrieb Zapped:
> Signed-off-by: Zapped <zapped@mail.ru>
> ---
>  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.<name>.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?

Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Johannes Schindelin]

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

Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> 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=<kind> 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?

Re[2]: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Алексей Шумкин]

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?

Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Jens Lehmann]

Am 26.12.2010 20:14, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> 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.<name>.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=<kind> 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?

Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Johannes Schindelin]

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

Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Casey Dahlin]

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

Re[2]: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Алексей Крезов]

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 <zapped@mail.ru>
>> ---
>>  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.<name>.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?

Re[2]: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

[Алексей Шумкин]

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?

Re: [PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal

[Thomas Rast]

Zapped wrote:
> Signed-off-by: Zapped <zapped@mail.ru>

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: Алексей Крезов <zapped@mail.ru>

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 <trast@student.ethz.ch>

in a reroll if you fix the above.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re[2]: [PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal

[Алексей Шумкин]

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 <zapped@mail.ru>

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: Алексей Крезов <zapped@mail.ru>

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 <trast@student.ethz.ch>

TR> in a reroll if you fix the above.

Re: [PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal

[Thomas Rast]

Алексей Шумкин 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

Re: [PATCH 1/3] Fixes bug: git-diff: class methods are not detected in hunk headers for Pascal

[Thomas Rast]

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