Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
Vasco Almeida writes: >> We only update comment_line_char from the default "#" when the >> configured value is a single-byte character and we ignore incorrect >> values in the configuration file. So I think the patch you sent is >> correct after all. > > I am still not sure what version do we prefer. > > Check whether core.commentchar is a single character. If not, use '#' > as the $comment_line_char. This, plus special casing "auto". Picking the first byte is inconsistent with the current practice (the paragraph you quoted above), I think.
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
A Sáb, 10-12-2016 às 14:09 -0800, Junio C Hamano escreveu:
> We only update comment_line_char from the default "#" when the
> configured value is a single-byte character and we ignore incorrect
> values in the configuration file. So I think the patch you sent is
> correct after all.
I am still not sure what version do we prefer.
Check whether core.commentchar is a single character. If not, use '#'
as the $comment_line_char.
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ $comment_line_char = '#' if (length($comment_line_char) != 1);
+ return $comment_line_char;
+}
Check whether core.commentchar is a single character. If not, use the
first character of the core.commentchar value, mirroring the "rebase
-i" behavior introduced recently.
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ if (length($comment_line_char) != 1) {
+ # use first character
+ $comment_line_char = substr($comment_line_char, 0, 1);
+ }
+ return $comment_line_char;
+}
Or akin to what I had in the first patch related to handling 'auto'
value of core.commentchar configuration variable:
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ return $comment_line_char;
+}
Which assumes that the value of core.commentchar configuration variable
is either 'auto' or one single character, or the variable is not
defined.
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
Vasco Almeida writes:
> I wonder why this is important when Git errors out when
> core.commentChar is set to more than 1 characters or 0 characters.
I think it should be consistent with the way core.commentchar is
treated in the rest of the system, namely this bit from config.c:
if (!strcmp(var, "core.commentchar")) {
if (!value)
return config_error_nonbool(var);
else if (!strcasecmp(value, "auto"))
auto_comment_line_char = 1;
else if (value[0] && !value[1]) {
comment_line_char = value[0];
auto_comment_line_char = 0;
} else
return error("core.commentChar should only be one
character");
return 0;
}
And I think I misread this piece of code.
We only update comment_line_char from the default "#" when the
configured value is a single-byte character and we ignore incorrect
values in the configuration file. So I think the patch you sent is
correct after all.
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
A Sex, 09-12-2016 às 14:23 -0800, Junio C Hamano escreveu:
> > This is exactly the same issue I fixed for rebase -i recently.
>
> Yes, but the patch we see here punts "core.commentChar is not a
> single-byte single-letter--panic!" case differently. I think you
> did "just take the first one" in "rebase -i", which I think is more
> in line with the rest of the system, and this addition to Git.pm
> should do the same, I think.
I hope the changes below are in line with the rest of the system. If
so, I will send a new re-roll with them.
I wonder why this is important when Git errors out when
core.commentChar is set to more than 1 characters or 0 characters. Is
it just to be consistent with "rebase -i" changes introduced
by Johannes Schindelin?
I am not sure what does "if (length($comment_line_char) != 1)" check.
Whether it checks single-byte or single-letter or both...
-- >8 --
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3a6d846..4e0ab5a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1072,7 +1072,7 @@ sub edit_hunk_manually {
print $fh @$oldtext;
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') :
('+', '-');
- my $comment_line_char = Git::config("core.commentchar") ||
'#';
+ my $comment_line_char = Git::get_comment_line_char;
print $fh Git::comment_lines sprintf(__ <
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
Johannes Schindelin writes: > Hi Vasco, > > On Fri, 9 Dec 2016, Vasco Almeida wrote: > >> A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu: >> > The incremental update below looks sensible. We'd also want to >> > protect this codepath from a misconfigured two-or-more byte sequence >> > in core.commentchar, I would suspect, to be consistent. >> >> Are the below changes alright for what you propose? It just checks if >> the length of core.commentchar's value is 1, otherwise use '#' as the >> comment_line_char. >> As a note, when I set core.commentchar with "git config >> core.commentChar 'batata'", I get the following error message when I >> issue "git add -i": >> >> error: core.commentChar should only be one character >> fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6 > > This is exactly the same issue I fixed for rebase -i recently. Yes, but the patch we see here punts "core.commentChar is not a single-byte single-letter--panic!" case differently. I think you did "just take the first one" in "rebase -i", which I think is more in line with the rest of the system, and this addition to Git.pm should do the same, I think.
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
Hi Vasco, On Fri, 9 Dec 2016, Vasco Almeida wrote: > A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu: > > The incremental update below looks sensible. We'd also want to > > protect this codepath from a misconfigured two-or-more byte sequence > > in core.commentchar, I would suspect, to be consistent. > > Are the below changes alright for what you propose? It just checks if > the length of core.commentchar's value is 1, otherwise use '#' as the > comment_line_char. > As a note, when I set core.commentchar with "git config > core.commentChar 'batata'", I get the following error message when I > issue "git add -i": > > error: core.commentChar should only be one character > fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6 This is exactly the same issue I fixed for rebase -i recently. Good eyes, Dscho
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu:
> The incremental update below looks sensible. We'd also want to
> protect this codepath from a misconfigured two-or-more byte sequence
> in core.commentchar, I would suspect, to be consistent.
Are the below changes alright for what you propose? It just checks if
the length of core.commentchar's value is 1, otherwise use '#' as the
comment_line_char.
As a note, when I set core.commentchar with "git config
core.commentChar 'batata'", I get the following error message when I
issue "git add -i":
error: core.commentChar should only be one character
fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6
-- >8 --
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3a6d846..4e0ab5a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1072,7 +1072,7 @@ sub edit_hunk_manually {
print $fh @$oldtext;
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+',
'-');
- my $comment_line_char = Git::config("core.commentchar") || '#';
+ my $comment_line_char = Git::get_comment_line_char;
print $fh Git::comment_lines sprintf(__ <
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
Vasco Almeida writes:
> A Sex, 11-11-2016 às 11:45 -0100, Vasco Almeida escreveu:
>> +=item comment_lines ( STRING [, STRING... ])
>> +
>> +Comments lines following core.commentchar configuration.
>> +
>> +=cut
>> +
>> +sub comment_lines {
>> + my $comment_line_char = config("core.commentchar") || '#';
>> + return prefix_lines("$comment_line_char ", @_);
>> +}
>> +
>
> In light of the recent "Fix problems with rebase -i when
> core.commentchar is defined" [1], I realized that this patch does not
> handle the 'auto' value of core.commentchat configuration variable.
>
> I propose to do the patch below in the next re-roll.
>
> [1] http://www.mail-archive.com/git@vger.kernel.org/msg107818.html
The incremental update below looks sensible. We'd also want to
protect this codepath from a misconfigured two-or-more byte sequence
in core.commentchar, I would suspect, to be consistent.
> -- >8 --
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 3a6d846..8d33634 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1073,6 +1073,7 @@ sub edit_hunk_manually {
> my $is_reverse = $patch_mode_flavour{IS_REVERSE};
> my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+',
> '-');
> my $comment_line_char = Git::config("core.commentchar") || '#';
> + $comment_line_char = '#' if ($comment_line_char eq 'auto');
> print $fh Git::comment_lines sprintf(__ < $remove_plus, $comment_line_char),
> ---
> To remove '%s' lines, make them ' ' lines (context).
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 69cd1dd..47b5899 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -1459,6 +1459,7 @@ Comments lines following core.commentchar configuration.
>
> sub comment_lines {
> my $comment_line_char = config("core.commentchar") || '#';
> + $comment_line_char = '#' if ($comment_line_char eq 'auto');
> return prefix_lines("$comment_line_char ", @_);
> }
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
A Sex, 11-11-2016 às 11:45 -0100, Vasco Almeida escreveu:
> +=item comment_lines ( STRING [, STRING... ])
> +
> +Comments lines following core.commentchar configuration.
> +
> +=cut
> +
> +sub comment_lines {
> + my $comment_line_char = config("core.commentchar") || '#';
> + return prefix_lines("$comment_line_char ", @_);
> +}
> +
In light of the recent "Fix problems with rebase -i when
core.commentchar is defined" [1], I realized that this patch does not
handle the 'auto' value of core.commentchat configuration variable.
I propose to do the patch below in the next re-roll.
[1] http://www.mail-archive.com/git@vger.kernel.org/msg107818.html
-- >8 --
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3a6d846..8d33634 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1073,6 +1073,7 @@ sub edit_hunk_manually {
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+',
'-');
my $comment_line_char = Git::config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
print $fh Git::comment_lines sprintf(__ <
[PATCH v6 01/16] Git.pm: add subroutines for commenting lines
Add subroutines prefix_lines and comment_lines.
Signed-off-by: Vasco Almeida
---
perl/Git.pm | 24
1 file changed, 24 insertions(+)
diff --git a/perl/Git.pm b/perl/Git.pm
index b2732822a..69cd1ddec 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1438,6 +1438,30 @@ sub END {
} # %TEMP_* Lexical Context
+=item prefix_lines ( PREFIX, STRING [, STRING... ])
+
+Prefixes lines in C with C.
+
+=cut
+
+sub prefix_lines {
+ my $prefix = shift;
+ my $string = join("\n", @_);
+ $string =~ s/^/$prefix/mg;
+ return $string;
+}
+
+=item comment_lines ( STRING [, STRING... ])
+
+Comments lines following core.commentchar configuration.
+
+=cut
+
+sub comment_lines {
+ my $comment_line_char = config("core.commentchar") || '#';
+ return prefix_lines("$comment_line_char ", @_);
+}
+
=back
=head1 ERROR HANDLING
--
2.11.0.rc0.33.gec17dab
