Re: [PATCH v3] send-email: extract email-parsing code into a subroutine

[Matthieu Moy]

PAYRE NATHAN p1508475  writes:

> Without the "print" used for testing. 

But still smoe broken indentation:

>  git-send-email.perl | 90 
> +
>  1 file changed, 63 insertions(+), 27 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2208dcc21..a10574a56 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -715,41 +715,63 @@ EOT3
>   if (!defined $compose_encoding) {
>   $compose_encoding = "UTF-8";
>   }
> - while(<$c>) {
> - next if m/^GIT:/;
> - if ($in_body) {
> - $summary_empty = 0 unless (/^\n$/);
> - } elsif (/^\n$/) {
> - $in_body = 1;
> - if ($need_8bit_cte) {
> +
> +my %parsed_email;
> + $parsed_email{'body'} = '';
> +while (my $line = <$c>) {
> + next if $line =~ m/^GIT:/;
> + parse_header_line($line, \%parsed_email);
> + if ($line =~ /^\n$/i) {
> + while (my $body_line = <$c>) {
> +if ($body_line !~ m/^GIT:/) {
> +$parsed_email{'body'} = $parsed_email{'body'} . 
> $body_line;
> +}
> + }
> + }
> + }

This may display properly in your text editor with your setting, but
appears broken at least with tab-width=8. Don't mix tabs and spaces. The
Git coding style is to indent with tabs.

To see what I mean, open the script in Emacs and type M-x
whitespace-mode RET.

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH v2] send-email: extract email-parsing code into a subroutine

[Eric Sunshine]

On Wed, Dec 6, 2017 at 5:55 PM, Nathan PAYRE  wrote:
> Junio C Hamano : writes:
>> Also "reusable in other place" is by itself not an unconditional
>> plus, until readers can be convinced that that 'other place' really
>> wants to be able to call this function.  Is there some untold
>> motivation behind this change---such as a planned update to actually
>> use this helper subroutine?
>
> This subroutine will be used to implement, initially a new option called
> "--quote-email", but became "--cite" added after "--in-reply-to".
> This will permit to the user to cite a mail and reply with a patch and keep
> Cc, To ...
> See discussion :
> https://public-inbox.org/git/20171030223444.5052-1-nathan.pa...@etu.univ-lyon1.fr/

Although he didn't state so explicitly, please take Junio's question
as a strong hint that you should update the commit message to include
the above rationale/explanation about why you consider this a good
change. The reason for including the motivation in the commit message
is that you want, not only to convince Junio now that this is a useful
change, but to convince future readers of the project history that
this change is desirable.

Re: [PATCH 3/3] diff: use skip_to_opt_val()

[Christian Couder]

On Thu, Dec 7, 2017 at 1:18 AM, Jacob Keller  wrote:
> On Wed, Dec 6, 2017 at 4:16 PM, Jacob Keller  wrote:
>>
>> This causes a regression in the --relative option which prevents it
>> from working properly.
>>
>> If I have a repository with a modified file in a subdirectory, such as:
>>
>> a/file
>>
>> then git diff-index --relative --name-only HEAD from within "a" will
>> return "a/file" instead of "file"
>>
>> This breaks git completion, (among other things).

Ok, thanks for the report. I will fix that.

> I believe this occurs because skip_to_optional_val overwrites the arg
> even if it's not there, and the --relative argument expects prefix to
> remain unchanged if the optional value is not provided.

Yeah, that makes sense.

Re: partial_clone_get_default_filter_spec has no callers

[Ramsay Jones]

On 06/12/17 21:07, Jeff Hostetler wrote:
> 
> 
> On 12/6/2017 12:39 PM, Ramsay Jones wrote:
>> Hi Jeff,
>>
>> commit f1862e8153 ("partial-clone: define partial clone settings
>> in config", 2017-12-05), which is part of your 'jh/partial-clone'
>> branch, introduces the partial_clone_get_default_filter_spec()
>> function without any callers. Could you please confirm that this
>> is intentional and that, presumably, a future series will include
>> a call to this function.
> 
> I'll double check.  Thanks.
> 
> BTW is there another tool that you're using to find these?
> I know I ran make DEVELOPER=1 and make sparse on everything
> and didn't see that come up.

In addition to sparse (which finds some of these), I also run a perl
script over the object files after a given build. (The script was
posted to the list by Junio, many moons ago, and I have made several
changes to my local copy).

I am attaching a copy of the script (static-check.pl). Note that the
'stop list' in the script (%def_ok) is _way_ out of date. However, the
way I use the script, that does not matter; I run the script over the
master->next->pu branches and (ignoring the master branch) diff the
result files from branch to branch. For example, tonight I have:

  $ wc -l sc nsc psc
74 sc
73 nsc
75 psc
   222 total
  $ 
  $ diff sc nsc
  44d43
  < oidmap.o- oidmap_remove
  $ 
  $ diff nsc psc
  43a44
  > list-objects-filter-options.o   - partial_clone_get_default_filter_spec
  58a60
  > sequencer.o - sign_off_header
  $ 

You also have to be careful with leaving stale object files
laying around from previous builds ('make clean' sometimes
doesn't). Actually, it may be simpler to read a previous mailing
list thread on exactly this subject [1].

BTW, if you are using a version of sparse post v0.5.1, you can
get rid of the only sparse warning on Linux (assuming you don't
build with NO_REGEX set), by using the -Wno-memcpy-max-count option;
I have the following set in my config.mak:

  $ tail -2 config.mak
  pack-revindex.sp: SPARSE_FLAGS += -Wno-memcpy-max-count

  $ 

[I haven't sent a proper patch, since the required version of
sparse is not widely available yet.]

ATB,
Ramsay Jones

[1] 
https://public-inbox.org/git/%3cb21c8a92-4dd5-56d6-ec6a-5709028ea...@ramsayjones.plus.com%3E/


static-check.pl
Description: Perl program

Re: [PATCH] diff: add test showing regression to --relative

[Jacob Keller]

On Wed, Dec 6, 2017 at 5:04 PM, Jeff King  wrote:
> On Wed, Dec 06, 2017 at 04:35:17PM -0800, Jacob Keller wrote:
>
>> Subject: [PATCH] diff: add test showing regression to --relative
>
> Since we'd hopefully not ever merge that regression, I think this patch
> ought to stand on its own. In which case it probably wants to say
> something like:
>
>   diff: test --relative without a prefix
>
>   We already test "diff --relative=subdir", but not that
>   "--relative" by itself should use the current directory as
>   its prefix.
>

Yea, I wasn't sure what the actual status of the changes were though,
so I thought I'd send the actual fix I did. It's definitely up to
Christian in determining what he thinks the best path forward is.

>> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
>> index 3950f5034d31..41e4f59b2ffb 100755
>> --- a/t/t4045-diff-relative.sh
>> +++ b/t/t4045-diff-relative.sh
>> @@ -70,4 +70,9 @@ for type in diff numstat stat raw; do
>>   check_$type dir/file2 --relative=sub
>>  done
>>
>> +cd subdir
>> +for type in diff numstat stat raw; do
>> + check_$type file2 --relative
>> +done
>
> We should avoid moving the cwd of the whole test script in
> case we add tests later. Normally we'd do the cd inside a
> subshell, but that's complicated by the wrapper (we wouldn't
> want to increment the test counter just inside the subshell,
> for instance).
>
> Adding "cd .." is the smallest thing we could do to fix
> that. But I think the more robust solution is to actually
> teach the check_* helper about doing the "cd" inside the
> test_expect block. Or just pushing the helper down into the
> test block and living with repeating the
> "test_expect_success" parts for each call.


Yea, I tried cd inside the subshell and it didn't work, so I did this
as the quicker solution.

I think the best method would be to update the check_* helper
functions to take a dir parameter, and use that to do git -C "dir"
when calling the diff commands.

Thanks,
Jake

>
> -Peff

Re: git commit file completion recently broke

[Jeff King]

On Wed, Dec 06, 2017 at 05:04:02PM -0800, Jacob Keller wrote:

> What you outlined above is probably the best we can do. We could even
> add some extra helper which does that for us if we want.
> 
> I sent a patch that merely reverts the change to --relative and adds a
> test for now though.

Thanks. I'm fine with reverting --relative to its original form, or
having it do the NULL thing I mentioned.  Christian can decide, since
it's his series.

I do think we may want to take the test improvement as a separate patch.

-Peff

Re: git commit file completion recently broke

[Jacob Keller]

On Wed, Dec 6, 2017 at 4:56 PM, Jeff King  wrote:
> On Wed, Dec 06, 2017 at 04:38:29PM -0800, Jacob Keller wrote:
>
>> >> But nope, it looks like the culprit is f7923a5ece (diff: use
>> >> skip_to_optional_val(), 2017-12-04), which switched over parsing of
>> >> "--relative".
>> >
>> > Oh, actually, I guess I was half-right. It feeds >prefix as the
>> > "default", meaning that we overwrite it with the empty string. I don't
>> > think "--relative" works for the semantics of skip_to_optional_value,
>> > since it needs:
>> >
>> >   --relative=foo: set prefix to "foo"
>> >
>> >   --relative: leave prefix untouched
>> >
>> > -Peff
>>
>> Yep, and apparently our test suite completely lacked any tests of
>> --relative on its own.
>>
>> I've sent a patch to add some tests.
>
> Great. I was also saddened by our lack of tests.
>
>> I don't know the exact best way to fix this, I guess we could just
>> revert it the changes to relative... but maybe we could add or modify
>> the semantics of skip_to_optional_val()?? What if it was changed so
>> that it left the value alone if no value was provided? This would
>> require callers to pre-set the value they want as default, but that
>> would solve relative's problem.
>
> I think that would work for this case. But just looking at others from
> the same series, I think they'd get pretty awkward. For instance we now
> have:
>

That obviously won't work for any case which sues
skip_to_optional_val_default() (since these provide a default value to
give in case none is provided.

>   else if (!strcmp(arg, "--color))
> options->use_color = 1;
>   else if (skip_prefix(arg, "--color=", ))
> /* parse "arg" as colorbool */
>
> which became:
>
>   else if (skip_to_optional_val_default(arg, "--color", , "always"))
> /* parse "arg" as colorbool */
>
> How would that look with the "leave it alone instead of assigning a
> default" semantics? It gets pretty clumsy, because you have to
> pre-assign "always" to some pointer. But then we can't reuse "arg", so
> we end up with something more like:
>
>   const char *color_val = "always";
>   ...
>   else if (skip_to_optional_val(arg, "--color", _val))
>

It obviously wouldn't. The only sensible solution is to have
"skip_to_optional_val_something()" which does this new behavior.

Or, change skip_to_optional_val() behave this new way, but
skip_to_optional_val_default() behave in the current way.

> But we need one such "color_val" for every option we test for, and we
> have to set all of them up before any matches (because we don't know
> which one we'll actually match). Yuck.
>
> I think we'd do better to just assign NULL when there's "=", so we can
> tell the difference between "--relative", "--relative=", and
> "--relative=foo" (all of which are distinct).
>
> I think that's possible with the current scheme by doing:
>
>   else if (skip_to_optional_val_default(arg, "--relative", , NULL)) {
> options->flags.relative_name = 1;
> if (arg)
> options->prefix = arg;
>   }
>
> IOW, the problem isn't in the design of the skip function, but just how
> it was used in this particular case. I do think it may make sense for
> the "short" one to use NULL, like:
>
>   skip_to_optional_val(arg, "--relative, )
>
> but maybe some other callers would be more inconvenienced (they may have
> to current NULL back into the empty string if they want to string
> "--foo" the same as "--foo=").
>
> -Peff

What you outlined above is probably the best we can do. We could even
add some extra helper which does that for us if we want.

I sent a patch that merely reverts the change to --relative and adds a
test for now though.

Thanks,
Jake

Re: [PATCH] diff: add test showing regression to --relative

[Jeff King]

On Wed, Dec 06, 2017 at 04:35:17PM -0800, Jacob Keller wrote:

> Subject: [PATCH] diff: add test showing regression to --relative

Since we'd hopefully not ever merge that regression, I think this patch
ought to stand on its own. In which case it probably wants to say
something like:

  diff: test --relative without a prefix

  We already test "diff --relative=subdir", but not that
  "--relative" by itself should use the current directory as
  its prefix.

> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
> index 3950f5034d31..41e4f59b2ffb 100755
> --- a/t/t4045-diff-relative.sh
> +++ b/t/t4045-diff-relative.sh
> @@ -70,4 +70,9 @@ for type in diff numstat stat raw; do
>   check_$type dir/file2 --relative=sub
>  done
>  
> +cd subdir
> +for type in diff numstat stat raw; do
> + check_$type file2 --relative
> +done

We should avoid moving the cwd of the whole test script in
case we add tests later. Normally we'd do the cd inside a
subshell, but that's complicated by the wrapper (we wouldn't
want to increment the test counter just inside the subshell,
for instance).

Adding "cd .." is the smallest thing we could do to fix
that. But I think the more robust solution is to actually
teach the check_* helper about doing the "cd" inside the
test_expect block. Or just pushing the helper down into the
test block and living with repeating the
"test_expect_success" parts for each call.

-Peff

[PATCH v2] diff: fix --relative without arguments

[Jacob Keller]

From: Jacob Keller 

Recently, commit f7923a5ece00 ("diff: use skip_to_optional_val()",
2017-12-04) changed how we parsed some diff options, preferring
skip_to_optional_val instead of a more complex skip_prefix() setup.

Unfortunately, this breaks --relative, because of the semantics of
skip_to_optional_val. If no optional_val is given, then the function
puts the empty string "" into the arg. Unfortunately, relative passes in
options->prefix as the location for the optional argument, and expects
this value to remain unchanged if no optional value was given.

This results in breaking --relative when no arguments are provided. This
cascades into many failures of code flow that rely on this.

Partially revert the commit, and restore --relative option parsing to
the previous working code.

Since no tests exist for --relative without options, add a new test
which will help ensure no future regressions.

Signed-off-by: Jacob Keller 
---
Actually perform the revert of --relative handling.

 diff.c   | 6 +-
 t/t4045-diff-relative.sh | 5 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index b62e4b61b564..1919afb4f104 100644
--- a/diff.c
+++ b/diff.c
@@ -4581,8 +4581,12 @@ int diff_opt_parse(struct diff_options *options,
options->flags.rename_empty = 1;
else if (!strcmp(arg, "--no-rename-empty"))
options->flags.rename_empty = 0;
-   else if (skip_to_optional_val(arg, "--relative", >prefix))
+   else if (!strcmp(arg, "--relative"))
options->flags.relative_name = 1;
+   else if (skip_prefix(arg, "--relative=", )) {
+   options->flags.relative_name = 1;
+   options->prefix = arg;
+   }
 
/* xdiff options */
else if (!strcmp(arg, "--minimal"))
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 3950f5034d31..41e4f59b2ffb 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -70,4 +70,9 @@ for type in diff numstat stat raw; do
check_$type dir/file2 --relative=sub
 done
 
+cd subdir
+for type in diff numstat stat raw; do
+   check_$type file2 --relative
+done
+
 test_done
-- 
2.15.1.477.g3ed0a2a61da8

Re: git commit file completion recently broke

[Jeff King]

On Wed, Dec 06, 2017 at 04:38:29PM -0800, Jacob Keller wrote:

> >> But nope, it looks like the culprit is f7923a5ece (diff: use
> >> skip_to_optional_val(), 2017-12-04), which switched over parsing of
> >> "--relative".
> >
> > Oh, actually, I guess I was half-right. It feeds >prefix as the
> > "default", meaning that we overwrite it with the empty string. I don't
> > think "--relative" works for the semantics of skip_to_optional_value,
> > since it needs:
> >
> >   --relative=foo: set prefix to "foo"
> >
> >   --relative: leave prefix untouched
> >
> > -Peff
> 
> Yep, and apparently our test suite completely lacked any tests of
> --relative on its own.
> 
> I've sent a patch to add some tests.

Great. I was also saddened by our lack of tests.

> I don't know the exact best way to fix this, I guess we could just
> revert it the changes to relative... but maybe we could add or modify
> the semantics of skip_to_optional_val()?? What if it was changed so
> that it left the value alone if no value was provided? This would
> require callers to pre-set the value they want as default, but that
> would solve relative's problem.

I think that would work for this case. But just looking at others from
the same series, I think they'd get pretty awkward. For instance we now
have:

  else if (!strcmp(arg, "--color))
options->use_color = 1;
  else if (skip_prefix(arg, "--color=", ))
/* parse "arg" as colorbool */

which became:

  else if (skip_to_optional_val_default(arg, "--color", , "always"))
/* parse "arg" as colorbool */

How would that look with the "leave it alone instead of assigning a
default" semantics? It gets pretty clumsy, because you have to
pre-assign "always" to some pointer. But then we can't reuse "arg", so
we end up with something more like:

  const char *color_val = "always";
  ...
  else if (skip_to_optional_val(arg, "--color", _val))

But we need one such "color_val" for every option we test for, and we
have to set all of them up before any matches (because we don't know
which one we'll actually match). Yuck.

I think we'd do better to just assign NULL when there's "=", so we can
tell the difference between "--relative", "--relative=", and
"--relative=foo" (all of which are distinct).

I think that's possible with the current scheme by doing:

  else if (skip_to_optional_val_default(arg, "--relative", , NULL)) {
options->flags.relative_name = 1;
if (arg)
options->prefix = arg;
  }

IOW, the problem isn't in the design of the skip function, but just how
it was used in this particular case. I do think it may make sense for
the "short" one to use NULL, like:

  skip_to_optional_val(arg, "--relative, )

but maybe some other callers would be more inconvenienced (they may have
to current NULL back into the empty string if they want to string
"--foo" the same as "--foo=").

-Peff

Re: git commit file completion recently broke

[Jacob Keller]

On Wed, Dec 6, 2017 at 4:24 PM, Jeff King  wrote:
> On Wed, Dec 06, 2017 at 07:22:35PM -0500, Jeff King wrote:
>
>> On Wed, Dec 06, 2017 at 04:01:51PM -0800, Jacob Keller wrote:
>>
>> > I think I narrowed this down to "git diff-index --name-only --relative
>> > HEAD" producing a list of files *not* relative to the current
>> > directory.
>>
>> Hmm, my guess would have been something funny in the setup code
>> forgetting our original prefix.
>>
>> But nope, it looks like the culprit is f7923a5ece (diff: use
>> skip_to_optional_val(), 2017-12-04), which switched over parsing of
>> "--relative".
>
> Oh, actually, I guess I was half-right. It feeds >prefix as the
> "default", meaning that we overwrite it with the empty string. I don't
> think "--relative" works for the semantics of skip_to_optional_value,
> since it needs:
>
>   --relative=foo: set prefix to "foo"
>
>   --relative: leave prefix untouched
>
> -Peff

Yep, and apparently our test suite completely lacked any tests of
--relative on its own.

I've sent a patch to add some tests.

I don't know the exact best way to fix this, I guess we could just
revert it the changes to relative... but maybe we could add or modify
the semantics of skip_to_optional_val()?? What if it was changed so
that it left the value alone if no value was provided? This would
require callers to pre-set the value they want as default, but that
would solve relative's problem.

I'll look into that.

Thanks,
Jake

[PATCH] diff: add test showing regression to --relative

[Jacob Keller]

From: Jacob Keller 

Signed-off-by: Jacob Keller 
---
 t/t4045-diff-relative.sh | 5 +
 1 file changed, 5 insertions(+)

diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 3950f5034d31..41e4f59b2ffb 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -70,4 +70,9 @@ for type in diff numstat stat raw; do
check_$type dir/file2 --relative=sub
 done
 
+cd subdir
+for type in diff numstat stat raw; do
+   check_$type file2 --relative
+done
+
 test_done
-- 
2.15.1.477.g3ed0a2a61da8

Re: git commit file completion recently broke

[Jeff King]

On Wed, Dec 06, 2017 at 07:22:35PM -0500, Jeff King wrote:

> On Wed, Dec 06, 2017 at 04:01:51PM -0800, Jacob Keller wrote:
> 
> > I think I narrowed this down to "git diff-index --name-only --relative
> > HEAD" producing a list of files *not* relative to the current
> > directory.
> 
> Hmm, my guess would have been something funny in the setup code
> forgetting our original prefix.
> 
> But nope, it looks like the culprit is f7923a5ece (diff: use
> skip_to_optional_val(), 2017-12-04), which switched over parsing of
> "--relative".

Oh, actually, I guess I was half-right. It feeds >prefix as the
"default", meaning that we overwrite it with the empty string. I don't
think "--relative" works for the semantics of skip_to_optional_value,
since it needs:

  --relative=foo: set prefix to "foo"

  --relative: leave prefix untouched

-Peff

Re: git commit file completion recently broke

[Jeff King]

On Wed, Dec 06, 2017 at 04:01:51PM -0800, Jacob Keller wrote:

> I think I narrowed this down to "git diff-index --name-only --relative
> HEAD" producing a list of files *not* relative to the current
> directory.

Hmm, my guess would have been something funny in the setup code
forgetting our original prefix.

But nope, it looks like the culprit is f7923a5ece (diff: use
skip_to_optional_val(), 2017-12-04), which switched over parsing of
"--relative".

-Peff

Re: [PATCH 3/3] diff: use skip_to_opt_val()

[Jacob Keller]

On Wed, Dec 6, 2017 at 4:16 PM, Jacob Keller  wrote:
> On Sun, Dec 3, 2017 at 9:04 AM, Christian Couder
>  wrote:
>> From: Christian Couder 
>>
>> Let's simplify diff option parsing using skip_to_opt_val().
>>
>> Signed-off-by: Christian Couder 
>> ---
>>  diff.c | 22 --
>>  1 file changed, 8 insertions(+), 14 deletions(-)
>>
>> diff --git a/diff.c b/diff.c
>> index 2ebe2227b4..067b498187 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -4508,17 +4508,11 @@ int diff_opt_parse(struct diff_options *options,
>> options->output_format |= DIFF_FORMAT_NUMSTAT;
>> else if (!strcmp(arg, "--shortstat"))
>> options->output_format |= DIFF_FORMAT_SHORTSTAT;
>> -   else if (!strcmp(arg, "-X") || !strcmp(arg, "--dirstat"))
>> -   return parse_dirstat_opt(options, "");
>> -   else if (skip_prefix(arg, "-X", ))
>> -   return parse_dirstat_opt(options, arg);
>> -   else if (skip_prefix(arg, "--dirstat=", ))
>> +   else if (skip_prefix(arg, "-X", ) || skip_to_opt_val(arg, 
>> "--dirstat", ))
>> return parse_dirstat_opt(options, arg);
>> else if (!strcmp(arg, "--cumulative"))
>> return parse_dirstat_opt(options, "cumulative");
>> -   else if (!strcmp(arg, "--dirstat-by-file"))
>> -   return parse_dirstat_opt(options, "files");
>> -   else if (skip_prefix(arg, "--dirstat-by-file=", )) {
>> +   else if (skip_to_opt_val(arg, "--dirstat-by-file", )) {
>> parse_dirstat_opt(options, "files");
>> return parse_dirstat_opt(options, arg);
>> }
>> @@ -4540,13 +4534,13 @@ int diff_opt_parse(struct diff_options *options,
>> return stat_opt(options, av);
>>
>> /* renames options */
>> -   else if (starts_with(arg, "-B") || starts_with(arg, 
>> "--break-rewrites=") ||
>> -!strcmp(arg, "--break-rewrites")) {
>> +   else if (starts_with(arg, "-B") ||
>> +skip_to_opt_val(arg, "--break-rewrites", )) {
>> if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
>> return error("invalid argument to -B: %s", arg+2);
>> }
>> -   else if (starts_with(arg, "-M") || starts_with(arg, 
>> "--find-renames=") ||
>> -!strcmp(arg, "--find-renames")) {
>> +   else if (starts_with(arg, "-M") ||
>> +skip_to_opt_val(arg, "--find-renames", )) {
>> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
>> return error("invalid argument to -M: %s", arg+2);
>> options->detect_rename = DIFF_DETECT_RENAME;
>> @@ -4554,8 +4548,8 @@ int diff_opt_parse(struct diff_options *options,
>> else if (!strcmp(arg, "-D") || !strcmp(arg, 
>> "--irreversible-delete")) {
>> options->irreversible_delete = 1;
>> }
>> -   else if (starts_with(arg, "-C") || starts_with(arg, 
>> "--find-copies=") ||
>> -!strcmp(arg, "--find-copies")) {
>> +   else if (starts_with(arg, "-C") ||
>> +skip_to_opt_val(arg, "--find-copies", )) {
>> if (options->detect_rename == DIFF_DETECT_COPY)
>> options->flags.find_copies_harder = 1;
>> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
>> --
>> 2.15.1.271.g1a4e40aa5d.dirty
>>
>
> This causes a regression in the --relative option which prevents it
> from working properly.
>
> If I have a repository with a modified file in a subdirectory, such as:
>
> a/file
>
> then git diff-index --relative --name-only HEAD from within "a" will
> return "a/file" instead of "file"
>
> This breaks git completion, (among other things).
>
> Thanks,
> Jake

I believe this occurs because skip_to_optional_val overwrites the arg
even if it's not there, and the --relative argument expects prefix to
remain unchanged if the optional value is not provided.

Thanks,
Jake

Re: [PATCH 3/3] diff: use skip_to_opt_val()

[Jacob Keller]

On Sun, Dec 3, 2017 at 9:04 AM, Christian Couder
 wrote:
> From: Christian Couder 
>
> Let's simplify diff option parsing using skip_to_opt_val().
>
> Signed-off-by: Christian Couder 
> ---
>  diff.c | 22 --
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 2ebe2227b4..067b498187 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4508,17 +4508,11 @@ int diff_opt_parse(struct diff_options *options,
> options->output_format |= DIFF_FORMAT_NUMSTAT;
> else if (!strcmp(arg, "--shortstat"))
> options->output_format |= DIFF_FORMAT_SHORTSTAT;
> -   else if (!strcmp(arg, "-X") || !strcmp(arg, "--dirstat"))
> -   return parse_dirstat_opt(options, "");
> -   else if (skip_prefix(arg, "-X", ))
> -   return parse_dirstat_opt(options, arg);
> -   else if (skip_prefix(arg, "--dirstat=", ))
> +   else if (skip_prefix(arg, "-X", ) || skip_to_opt_val(arg, 
> "--dirstat", ))
> return parse_dirstat_opt(options, arg);
> else if (!strcmp(arg, "--cumulative"))
> return parse_dirstat_opt(options, "cumulative");
> -   else if (!strcmp(arg, "--dirstat-by-file"))
> -   return parse_dirstat_opt(options, "files");
> -   else if (skip_prefix(arg, "--dirstat-by-file=", )) {
> +   else if (skip_to_opt_val(arg, "--dirstat-by-file", )) {
> parse_dirstat_opt(options, "files");
> return parse_dirstat_opt(options, arg);
> }
> @@ -4540,13 +4534,13 @@ int diff_opt_parse(struct diff_options *options,
> return stat_opt(options, av);
>
> /* renames options */
> -   else if (starts_with(arg, "-B") || starts_with(arg, 
> "--break-rewrites=") ||
> -!strcmp(arg, "--break-rewrites")) {
> +   else if (starts_with(arg, "-B") ||
> +skip_to_opt_val(arg, "--break-rewrites", )) {
> if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
> return error("invalid argument to -B: %s", arg+2);
> }
> -   else if (starts_with(arg, "-M") || starts_with(arg, 
> "--find-renames=") ||
> -!strcmp(arg, "--find-renames")) {
> +   else if (starts_with(arg, "-M") ||
> +skip_to_opt_val(arg, "--find-renames", )) {
> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
> return error("invalid argument to -M: %s", arg+2);
> options->detect_rename = DIFF_DETECT_RENAME;
> @@ -4554,8 +4548,8 @@ int diff_opt_parse(struct diff_options *options,
> else if (!strcmp(arg, "-D") || !strcmp(arg, "--irreversible-delete")) 
> {
> options->irreversible_delete = 1;
> }
> -   else if (starts_with(arg, "-C") || starts_with(arg, "--find-copies=") 
> ||
> -!strcmp(arg, "--find-copies")) {
> +   else if (starts_with(arg, "-C") ||
> +skip_to_opt_val(arg, "--find-copies", )) {
> if (options->detect_rename == DIFF_DETECT_COPY)
> options->flags.find_copies_harder = 1;
> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
> --
> 2.15.1.271.g1a4e40aa5d.dirty
>

This causes a regression in the --relative option which prevents it
from working properly.

If I have a repository with a modified file in a subdirectory, such as:

a/file

then git diff-index --relative --name-only HEAD from within "a" will
return "a/file" instead of "file"

This breaks git completion, (among other things).

Thanks,
Jake

Re: git commit file completion recently broke

[Jacob Keller]

On Wed, Dec 6, 2017 at 4:01 PM, Jacob Keller  wrote:
> On Wed, Dec 6, 2017 at 3:53 PM, Jacob Keller  wrote:
>> Hi,
>>
>> I'm still investigating, but thought I'd send an email. I recently
>> updated to jch branch, and found that completion for git commit does
>> not work as expected.
>>
>> If I have a git repository with a modified file in a subdirectiory,
>> then git commit  produces the name of the subdirectory instead of
>> the file names.
>>
>> This occurs regardless of where I run the git commit command.
>>
>> Thanks,
>> Jake
>
> I think I narrowed this down to "git diff-index --name-only --relative
> HEAD" producing a list of files *not* relative to the current
> directory.
>
> Thanks,
> Jake

I've started a git bisect to see if i can find the source of the problem.

Thanks,
Jake

Re: [PATCH v3] git-gui: Prevent double UTF-8 conversion

[Johannes Schindelin]

Hi Łukasz,

On Tue, 5 Dec 2017, Łukasz Stelmach wrote:

> Convert author's name and e-mail address from the UTF-8 (or any other)
> encoding in load_last_commit function the same way commit message is
> converted.
> 
> Amending commits in git-gui without such conversion breaks UTF-8
> strings. For example, "\305\201ukasz" (as written by git cat-file) becomes
> "\303\205\302\201ukasz" in an amended commit.
> 
> Signed-off-by: Łukasz Stelmach 

This patch looks good to me.

Thanks,
Johannes

Re: git commit file completion recently broke

[Jacob Keller]

On Wed, Dec 6, 2017 at 3:53 PM, Jacob Keller  wrote:
> Hi,
>
> I'm still investigating, but thought I'd send an email. I recently
> updated to jch branch, and found that completion for git commit does
> not work as expected.
>
> If I have a git repository with a modified file in a subdirectiory,
> then git commit  produces the name of the subdirectory instead of
> the file names.
>
> This occurs regardless of where I run the git commit command.
>
> Thanks,
> Jake

I think I narrowed this down to "git diff-index --name-only --relative
HEAD" producing a list of files *not* relative to the current
directory.

Thanks,
Jake

git commit file completion recently broke

[Jacob Keller]

Hi,

I'm still investigating, but thought I'd send an email. I recently
updated to jch branch, and found that completion for git commit does
not work as expected.

If I have a git repository with a modified file in a subdirectiory,
then git commit  produces the name of the subdirectory instead of
the file names.

This occurs regardless of where I run the git commit command.

Thanks,
Jake

Re: [PATCH v3] send-email: extract email-parsing code into a subroutine

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 7, 2017 at 12:02 AM, Nathan Payre
 wrote:

> +sub parse_header_line {
> +   my $lines = shift;
> +   my $parsed_line = shift;
> +
> +   foreach (split(/\n/, $lines)) {
> +   if (/^(To|Cc|Bcc):\s*(.+)$/i) {
> +   $parsed_line->{lc $1} = [ parse_address_line($2) ];
> +   } elsif 
> (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|Content-Transfer-Encoding|References):\s*(.+)\s*$/i)
>  {
> +   $parsed_line->{lc $1} = $2;
> +   }
> +   }
> +}

Nit: As noted in my earlier review this results in very long lines,
I'm just typing this pseudocode into an E-Mail client so not tested at
all, but this would be better:

   my $header_parsed   = join "|", qw(To Cc Bcc);
   my $header_unparsed = join "|", qw(From Subject Message-ID ...); #
line wrap this at some point
   foreach [...]
   if (/^($header_parsed)[...]
   } elsif (^/($header_unparsed)[...].

Re: [PATCH v2] send-email: extract email-parsing code into a subroutine

[Junio C Hamano]

Nathan PAYRE  writes:

> Junio C Hamano : writes:
>
>> ... throughout this patch, not limited to this section, indentation
>> is strange and there seem to be many "print" that show messages that
>> do not seem to be meant for end-user consumption.  I can see that
>> this aspires to improve the readability, but not quite yet ;-).
>
> Hmmm I'm wondering who place thoses print in my code !
> I will fix it fast. :-)

Don't make waste by being hasty, though.  The print statements were
bad, but funny indentation was more distracting and will be worse
hindrance from the maintainabaility's point of view.

Thanks.

[PATCH v3] send-email: extract email-parsing code into a subroutine

[Nathan Payre]

The existing code mixes parsing of email header with regular
expression and actual code. Extract the parsing code into a new
subroutine 'parse_header_line()'. This improves the code readability
and make parse_header_line reusable in other place.

Signed-off-by: Nathan Payre 
Signed-off-by: Matthieu Moy 
Signed-off-by: Timothee Albertin 
Signed-off-by: Daniel Bensoussan 
Thanks-to: Ævar Arnfjörð Bjarmason 
---

Without the "print" used for testing. 

 git-send-email.perl | 90 +
 1 file changed, 63 insertions(+), 27 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2208dcc21..a10574a56 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -715,41 +715,63 @@ EOT3
if (!defined $compose_encoding) {
$compose_encoding = "UTF-8";
}
-   while(<$c>) {
-   next if m/^GIT:/;
-   if ($in_body) {
-   $summary_empty = 0 unless (/^\n$/);
-   } elsif (/^\n$/) {
-   $in_body = 1;
-   if ($need_8bit_cte) {
+
+my %parsed_email;
+   $parsed_email{'body'} = '';
+while (my $line = <$c>) {
+   next if $line =~ m/^GIT:/;
+   parse_header_line($line, \%parsed_email);
+   if ($line =~ /^\n$/i) {
+   while (my $body_line = <$c>) {
+if ($body_line !~ m/^GIT:/) {
+$parsed_email{'body'} = $parsed_email{'body'} . $body_line;
+}
+   }
+   }
+   }
+
+   if ($parsed_email{'from'}) {
+   $sender = $parsed_email{'from'};
+   }
+   if ($parsed_email{'in-reply-to'}) {
+   $initial_reply_to = $parsed_email{'in-reply-to'};
+   }
+   if ($parsed_email{'subject'}) {
+   $initial_subject = $parsed_email{'subject'};
+   print $c2 "Subject: " .
+   quote_subject($parsed_email{'subject'}, 
$compose_encoding) .
+   "\n";
+   }
+   if ($parsed_email{'mime-version'}) {
+   $need_8bit_cte = 0;
+   print $c2 "MIME-Version: $parsed_email{'mime-version'}\n",
+   "Content-Type: 
$parsed_email{'content-type'};\n",
+   "Content-Transfer-Encoding: 
$parsed_email{'content-transfer-encoding'}\n";
+   }
+   if ($need_8bit_cte) {
+   if ($parsed_email{'content-type'}) {
+   print $c2 "MIME-Version: 1.0\n",
+"Content-Type: 
$parsed_email{'content-type'};",
+"Content-Transfer-Encoding: 8bit\n";
+   } else {
print $c2 "MIME-Version: 1.0\n",
 "Content-Type: text/plain; ",
-  "charset=$compose_encoding\n",
+"charset=$compose_encoding\n",
 "Content-Transfer-Encoding: 8bit\n";
}
-   } elsif (/^MIME-Version:/i) {
-   $need_8bit_cte = 0;
-   } elsif (/^Subject:\s*(.+)\s*$/i) {
-   $initial_subject = $1;
-   my $subject = $initial_subject;
-   $_ = "Subject: " .
-   quote_subject($subject, $compose_encoding) .
-   "\n";
-   } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
-   $initial_reply_to = $1;
-   next;
-   } elsif (/^From:\s*(.+)\s*$/i) {
-   $sender = $1;
-   next;
-   } elsif (/^(?:To|Cc|Bcc):/i) {
-   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
-   next;
-   }
-   print $c2 $_;
}
+   if ($parsed_email{'body'}) {
+   $summary_empty = 0;
+   print $c2 "\n$parsed_email{'body'}\n";
+   }
+
close $c;
close $c2;
 
+   open $c2, "<", $compose_filename . ".final"
+   or die sprintf(__("Failed to open %s.final: %s"), 
$compose_filename, $!);
+   close $c2;
+
if ($summary_empty) {
print __("Summary email is empty, skipping it\n");
$compose = -1;
@@ -792,6 +814,20 @@ sub ask {
return;
 }
 
+sub parse_header_line {
+   my $lines = shift;
+   my $parsed_line = shift;
+
+   foreach (split(/\n/, $lines)) {
+   if (/^(To|Cc|Bcc):\s*(.+)$/i) {
+   $parsed_line->{lc $1} = [ parse_address_line($2) ];

Re: [PATCH v2] send-email: extract email-parsing code into a subroutine

[Nathan PAYRE]

Junio C Hamano : writes:

> ... throughout this patch, not limited to this section, indentation
> is strange and there seem to be many "print" that show messages that
> do not seem to be meant for end-user consumption.  I can see that
> this aspires to improve the readability, but not quite yet ;-).

Hmmm I'm wondering who place thoses print in my code !
I will fix it fast. :-)

> Also "reusable in other place" is by itself not an unconditional
> plus, until readers can be convinced that that 'other place' really
> wants to be able to call this function.  Is there some untold
> motivation behind this change---such as a planned update to actually
> use this helper subroutine?

This subroutine will be used to implement, initially a new option called
"--quote-email", but became "--cite" added after "--in-reply-to".
This will permit to the user to cite a mail and reply with a patch and keep
Cc, To ...
See discussion :
https://public-inbox.org/git/20171030223444.5052-1-nathan.pa...@etu.univ-lyon1.fr/

And Daniel Timothee and I wanted to refactor an other part of the file
using parse_header_line(). Near Line 1570.

Re: [WIP 07/15] connect: convert get_remote_heads to use struct packet_reader

[Junio C Hamano]

Brandon Williams  writes:

> @@ -56,6 +62,41 @@ static void die_initial_contact(int unexpected)
> "and the repository exists."));
>  }
>  
> +static enum protocol_version discover_version(struct packet_reader *reader)
> +{
> + enum protocol_version version = protocol_unknown_version;
> +
> + /*
> +  * Peek the first line of the server's response to
> +  * determine the protocol version the server is speaking.
> +  */
> + switch (packet_reader_peek(reader)) {
> + case PACKET_READ_ERROR:
> + die_initial_contact(0);
> + case PACKET_READ_FLUSH:
> + case PACKET_READ_DELIM:
> + version = protocol_v0;
> + break;
> + case PACKET_READ_NORMAL:
> + version = determine_protocol_version_client(reader->line);
> + break;
> + }
> +
> + /* Maybe process capabilities here, at least for v2 */
> + switch (version) {
> + case protocol_v1:
> + /* Read the peeked version line */
> + packet_reader_read(reader);
> + break;
> + case protocol_v0:
> + break;
> + case protocol_unknown_version:
> + BUG("ERROR");
> + }
> +
> + return version;
> +}
> +

This discovers and consumes the "version" thing, but for an older
protocol that does not have the "version" thing, it does not clobber
the first "ref", thanks to its use of peek.  Makes sense.

> +#define EXPECTING_FIRST_REF 0
> +#define EXPECTING_REF 1
> +#define EXPECTING_SHALLOW 2
> +#define EXPECTING_DONE 3
>  
>  static void process_capabilities(int *len)
>  {
> @@ -230,28 +237,34 @@ struct ref **get_remote_heads(int in, char *src_buf, 
> size_t src_len,
> struct oid_array *shallow_points)
>  {
>   struct ref **orig_list = list;
> + int len = 0;
> + int state = EXPECTING_FIRST_REF;
> + struct packet_reader reader;
> + const char *arg;
>  
> - /*
> -  * A hang-up after seeing some response from the other end
> -  * means that it is unexpected, as we know the other end is
> -  * willing to talk to us.  A hang-up before seeing any
> -  * response does not necessarily mean an ACL problem, though.
> -  */
> - int responded = 0;
> - int len;
> - int state = EXPECTING_PROTOCOL_VERSION;
> + packet_reader_init(, in, src_buf, src_len);
> +
> + discover_version();
>  
>   *list = NULL;

And thanks to the "peeking" implementation, the version handling is
cleanly separated from the rest of the exchange, which is good.

> - while ((len = read_remote_ref(in, _buf, _len, ))) {
> + while (state != EXPECTING_DONE) {
> + switch (packet_reader_read()) {
> + case PACKET_READ_ERROR:
> + die_initial_contact(1);
> + case PACKET_READ_NORMAL:
> + len = reader.pktlen;
> + if (len > 4 && skip_prefix(packet_buffer, "ERR ", ))
> + die("remote error: %s", arg);
> + break;
> + case PACKET_READ_FLUSH:
> + state = EXPECTING_DONE;
> + break;
> + case PACKET_READ_DELIM:
> + die("invalid packet\n");
> + }
> +

EXPECTING_DONE sounded like we are expecting to see 'done' packet
sent from the other side, but I was mistaken.  It is the state
where we are "done" expecting anything ;-).

Having an (unconditional) assignment to 'state' in the above switch
makes me feel somewhat uneasy, as the next "switch (state)" is what
is meant as the state machine that would allow us to say things like
"from this state, transition to that state is impossible".  When we
get a flush while we are expecting the first ref, for example, we'd
just go into the "done" state.  There is no provision for a future
update to say "no, getting a flush in this state is an error".

That is no different from the current code; when read_remote_ref()
notices that it got a flush, it just leaves the loop without even
touching 'state' variable.  But at least, I find that the current
code is more honest---it does not even touch 'state' and allows the
code after the loop to inspect it, if needed.  From that point of
vhew, the way the new code uses 'state' to leave the loop upon
seeing a flush is a regression---it makes it harder to notice and
report when we got a flush in a wrong state.

Perhaps getting rid of "EXPECTING_DONE" from the enum and then:

int got_flush = 0;
while (1) {
switch (reader_read()) {
case PACKET_READ_FLUSH:
got_flush = 1;
break;
... other cases ...
}

if (got_flush)
break;

switch (state) {
... current code ...
}
}

would be an improvement; we can later 

Re: [WIP 06/15] transport: use get_refs_via_connect to get refs

[Junio C Hamano]

Brandon Williams  writes:

> Remove code duplication and use the existing 'get_refs_via_connect()'
> function to retrieve a remote's heads in 'fetch_refs_via_pack()' and
> 'git_transport_push()'.
>
> Signed-off-by: Brandon Williams 
> ---
>  transport.c | 18 --
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index d75ff0514..7c969f285 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport 
> *transport,
>   args.cloning = transport->cloning;
>   args.update_shallow = data->options.update_shallow;
>  
> - if (!data->got_remote_heads) {
> - connect_setup(transport, 0);
> - get_remote_heads(data->fd[0], NULL, 0, _tmp, 0,
> -  NULL, >shallow);
> - data->got_remote_heads = 1;
> - }
> + if (!data->got_remote_heads)
> + refs_tmp = get_refs_via_connect(transport, 0);

The updated version is equivalent to the original as long as
transport->data->extra_have is empty at this point.  Were we
deliberately sending NULL, instead of >extra_have, in the
original, or is it a mere oversight?

The same comment applies to the other hunk of this patch.

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Eric Sunshine]

On Wed, Dec 6, 2017 at 5:00 PM, Brandon Williams  wrote:
> On 12/05, Eric Sunshine wrote:
>> 557a5998d (submodule: remove gitmodules_config, 2017-08-03) touched a
>> fair number of built-in commands. It's not clear from the current
>> patch's commit message if diff-tree is the only command which
>> regressed. Is it? Or are other commands also likely to have regressed?
>> Perhaps the commit message could say something about this. For
>> instance: "All other commands touched by 557a5998d have been audited
>> and were found to be regression-free" or "Other commands may regress
>> in the same way, but we will take a wait-and-see attitude and fix them
>> as needed because ".
>
> I don't know if any other commands have regressed.  This was such an odd
> regression that I think it would be difficult to say for certain that
> there couldn't be others.  I did go through the affected builtins to see
> if I could find anything.  I came up empty handed so I think we should
> be ok.

Thanks for the response. It would be nice to have this explained in
the commit message so that future readers don't have to wonder about
it.

Re: [PATCH v7 4/7] checkout: describe_detached_head: remove ellipsis after committish

[Ann T Ropea]

Junio C Hamano  writes:

> I've however queued the following on top of the entire series.  I do
> not mind squashing it into this step, though.

> Subject: [PATCH] t2020: test variations that matter

That sounds and looks fine to me.  I'm going to do this on my
side (getting it from 'pu' once the squashing is done).

Thanks.

[PATCH v2] diff-tree: read the index so attribute checks work in bare repositories

[Brandon Williams]

A regression was introduced in 557a5998d (submodule: remove
gitmodules_config, 2017-08-03) to how attribute processing was handled
in bare repositories when running the diff-tree command.

By default the attribute system will first try to read ".gitattribute"
files from the working tree and then falls back to reading them from the
index if there isn't a copy checked out in the worktree.  Prior to
557a5998d the index was read as a side effect of the call to
'gitmodules_config()' which ensured that the index was already populated
before entering the attribute subsystem.

Since the call to 'gitmodules_config()' was removed the index is no
longer being read so when the attribute system tries to read from the
in-memory index it doesn't find any ".gitattribute" entries effectively
ignoring any configured attributes.

Fix this by explicitly reading the index during the setup of diff-tree.

Reported-by: Ben Boeckel 
Signed-off-by: Brandon Williams 
---
 builtin/diff-tree.c|  2 ++
 t/t4015-diff-whitespace.sh | 17 +
 2 files changed, 19 insertions(+)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index d66499909..b775a7564 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -110,6 +110,8 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
 
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
init_revisions(opt, prefix);
+   if (read_cache() < 0)
+   die(_("index file corrupt"));
opt->abbrev = 0;
opt->diff = 1;
opt->disable_stdin = 1;
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 559a7541a..17df491a3 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -636,6 +636,23 @@ test_expect_success 'check with space before tab in indent 
(diff-tree)' '
test_must_fail git diff-tree --check HEAD^ HEAD
 '
 
+test_expect_success 'check with ignored trailing whitespace attr (diff-tree)' '
+   test_when_finished "git reset --hard HEAD^" &&
+
+   # create a whitespace error that should be ignored
+   echo "* -whitespace" >.gitattributes &&
+   git add .gitattributes &&
+   echo "foo(); " >x &&
+   git add x &&
+   git commit -m "add trailing space" &&
+
+   # with a worktree diff-tree ignores the whitespace error
+   git diff-tree --root --check HEAD &&
+
+   # without a worktree diff-tree still ignores the whitespace error
+   git -C .git diff-tree --root --check HEAD
+'
+
 test_expect_success 'check trailing whitespace (trailing-space: off)' '
git config core.whitespace "-trailing-space" &&
echo "foo ();   " >x &&
-- 
2.15.1.424.g9478a66081-goog

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Brandon Williams]

On 12/05, Eric Sunshine wrote:
> On Tue, Dec 5, 2017 at 5:13 PM, Brandon Williams  wrote:
> > A regression was introduced in 557a5998d (submodule: remove
> > gitmodules_config, 2017-08-03) to how attribute processing was handled
> > in bare repositories when running the diff-tree command.
> >
> > By default the attribute system will first try to read ".gitattribute"
> > files from the working tree and then falls back to reading them from the
> > index if there isn't a copy checked out in the worktree.  Prior to
> > 557a5998d the index was read as a side effect of the call to
> > 'gitmodules_config()' which ensured that the index was already populated
> > before entering the attribute subsystem.
> >
> > Since the call to 'gitmodules_config()' was removed the index is no
> > longer being read so when the attribute system tries to read from the
> > in-memory index it doesn't find any ".gitattribute" entries effectively
> > ignoring any configured attributes.
> >
> > Fix this by explicitly reading the index during the setup of diff-tree.
> 
> This commit message does a good job of explaining the issue, so
> someone who hasn't followed the thread (or has not followed it
> closely, like me) can understand the problem and solution. Thanks.
> 
> A few comments below...
> 
> > Reported-by: Ben Boeckel 
> > Signed-off-by: Brandon Williams 
> > ---
> > diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> > @@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const 
> > char *prefix)
> >
> > git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> > init_revisions(opt, prefix);
> > +   read_cache();
> 
> 557a5998d (submodule: remove gitmodules_config, 2017-08-03) touched a
> fair number of built-in commands. It's not clear from the current
> patch's commit message if diff-tree is the only command which
> regressed. Is it? Or are other commands also likely to have regressed?
> Perhaps the commit message could say something about this. For
> instance: "All other commands touched by 557a5998d have been audited
> and were found to be regression-free" or "Other commands may regress
> in the same way, but we will take a wait-and-see attitude and fix them
> as needed because ".

I don't know if any other commands have regressed.  This was such an odd
regression that I think it would be difficult to say for certain that
there couldn't be others.  I did go through the affected builtins to see
if I could find anything.  I came up empty handed so I think we should
be ok.

> 
> > diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> > @@ -636,6 +636,23 @@ test_expect_success 'check with space before tab in 
> > indent (diff-tree)' '
> > +test_expect_success 'check with ignored trailing whitespace attr 
> > (diff-tree)' '
> > +   test_when_finished "git reset --hard HEAD^" &&
> 
> A few style nits...
> 
> > +   # Create a whitespace error that should be ignored.
> 
> Comments in nearby tests are not capitalized and do not end with period.
> 
> > +   echo "* -whitespace" > ".gitattributes" &&
> 
> Please drop unnecessary quotes around the filename, as the extra noise
> makes it a bit harder to read. Also, lose space after redirection
> operator:
> 
> echo "* -whitespace" >.gitattributes &&
> 
> > +   git add ".gitattributes" &&
> > +   echo "trailing space -> " > "trailing-space" &&
> 
> All the nearby tests use some variation of:
> 
> echo "foo ();  " >x &&
> 
> which differs from the "trailing space ->" and filename
> 'trailing-space' used in this test. Lack of consistency makes this new
> test stick out like a sore thumb.

You're right, when writing the tests I didn't really consider the
surrounding ones.  I'll make the requested changes.

> 
> > +   git add "trailing-space" &&
> > +   git commit -m "trailing space" &&
> > +
> > +   # With a worktree diff-tree ignores the whitespace error
> > +   git diff-tree --root --check HEAD &&
> > +
> > +   # Without a worktree diff-tree still ignores the whitespace error
> > +   git -C .git diff-tree --root --check HEAD
> > +'

-- 
Brandon Williams

Re: [WIP 04/15] upload-pack: convert to a builtin

[Junio C Hamano]

Brandon Williams  writes:

> In order to allow for code sharing with the server-side of fetch in
> protocol-v2 convert upload-pack to be a builtin.
>
> Signed-off-by: Brandon Williams 
> ---

This looks obvious and straight-forward to a cursory look.

I vaguely recalled and feared that we on purpose kept this program
separate from the rest of the system for a reason, but my mailing
list search is coming up empty.

>  Makefile  | 3 ++-
>  builtin.h | 1 +
>  git.c | 1 +
>  upload-pack.c | 2 +-
>  4 files changed, 5 insertions(+), 2 deletions(-)
> ...
> diff --git a/upload-pack.c b/upload-pack.c
> index ef99a029c..2d16952a3 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1033,7 +1033,7 @@ static int upload_pack_config(const char *var, const 
> char *value, void *unused)
>   return parse_hide_refs_config(var, value, "uploadpack");
>  }
>  
> -int cmd_main(int argc, const char **argv)
> +int cmd_upload_pack(int argc, const char **argv, const char *prefix)
>  {
>   const char *dir;
>   int strict = 0;

Shouldn't this file be moved to builtin/ directory, though?

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Brandon Williams]

On 12/05, Stefan Beller wrote:
> On Tue, Dec 5, 2017 at 2:13 PM, Brandon Williams  wrote:
> > A regression was introduced in 557a5998d (submodule: remove
> > gitmodules_config, 2017-08-03) to how attribute processing was handled
> > in bare repositories when running the diff-tree command.
> >
> > By default the attribute system will first try to read ".gitattribute"
> > files from the working tree and then falls back to reading them from the
> > index if there isn't a copy checked out in the worktree.  Prior to
> > 557a5998d the index was read as a side effect of the call to
> > 'gitmodules_config()' which ensured that the index was already populated
> > before entering the attribute subsystem.
> >
> > Since the call to 'gitmodules_config()' was removed the index is no
> > longer being read so when the attribute system tries to read from the
> > in-memory index it doesn't find any ".gitattribute" entries effectively
> > ignoring any configured attributes.
> >
> > Fix this by explicitly reading the index during the setup of diff-tree.
> >
> > Reported-by: Ben Boeckel 
> > Signed-off-by: Brandon Williams 
> > ---
> >
> > This patch should fix the regression.  Let me know if it doesn't solve the
> > issue and I'll investigate some more.
> >
> 
> Thanks for fixing this bug! The commit message is helpful
> to understand how this bug could slip in!
> 
> > diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> > index d66499909..cfe7d0281 100644
> > --- a/builtin/diff-tree.c
> > +++ b/builtin/diff-tree.c
> > @@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const 
> > char *prefix)
> >
> > git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> > init_revisions(opt, prefix);
> > +   read_cache();
> 
> 
> Although we do have very few unchecked calls to read_cache, I'd suggest
> to avoid spreading them. Most of the read_cache calls are guarded via:
> 
> if (read_cache() < 0)
> die(_("index file corrupt"));

Thanks, I'll add this change.

> 
> I wonder if this hints at a bad API, and we'd rather have read_cache
> die() on errors, and the few callers that try to get out of trouble might
> need to use read_cache_gently() instead.
> (While this potentially large refactoring may be deferred, I'd ask for
> an if at least)
> 
> Thanks,
> Stefan

-- 
Brandon Williams

Re: 'git worktree add' does not fire post-checkout hook

[Eric Sunshine]

On Wed, Dec 6, 2017 at 4:00 PM, Gumbel, Matthew K
 wrote:
> I've noticed that when I run 'git worktree add /path/to/new/tree',
> the post-checkout hook does not fire, even though the worktree
> manpage explicitly states that "worktree add" will, "Create 
> and checkout  into it."
>
> Is this the intended behavior? Seems like maybe a bug, but I'm not
> sure.

Seems like an oversight. Given that 'git worktree' is like a cross of
'git clone' and 'git checkout', both of which run that hook, it seems
reasonable that 'git-worktree' should do so, as well.

If you'd like to get your feet wet at contributing to the Git project,
this might be a good first dip, as it looks like an easy fix (a one-
or two-liner). The only thing which needs a bit of care is to skip the
hook when --no-checkout is specified. Other than that, 'githooks'
documentation would need an update to mention that git-worktree also
runs the hook, and t2025-worktree-add.sh would want a couple new tests
(which would probably be the most complex part of the patch).

Re: [RFC] 'unsigned long' to 'size_t' conversion

[Jeff King]

On Wed, Dec 06, 2017 at 10:08:23AM -0500, Derrick Stolee wrote:

> There are several places in Git where we refer to the size of an object by
> an 'unsigned long' instead of a 'size_t'. In 64-bit Linux, 'unsigned long'
> is 8 bytes, but in 64-bit Windows it is 4 bytes.
> 
> The main issue with this conversion is that large objects fail to load (they
> seem to hash and store just fine). For example, the following 'blob8gb' is
> an 8 GB file where the ith byte is equal to i % 256:

Yeah, I think there's widespread agreement that this needs fixing. It's
just big enough that nobody has tackled it. If you're willing, that
sounds great to me. ;)

> In my opinion, the correct thing to do would be to replace all 'unsigned
> long's that refer to an object size and replace them with 'size_t'. However,
> a simple "git grep 'unsigned long size'" reveals 194 results, and there are
> other permutations of names and pointer types all over.

Yep. I think it's actually even trickier than that, though. Something
like off_t is probably the right type for the abstract concept of an
object size, since objects can be larger than the address space (in
which case we'd generally hit streaming code paths when possible).

But then of course we'd want to use size_t for objects that we've
actually placed into a buffers. At first glance it might be OK to just
use a larger type everywhere, but I think that runs into some funny
situations. For instance, you would not want to pass an off_t to
xmalloc(), as it is truncated (which may lead to a too-small buffer and
then a heap-smashing overflow).

So in an ideal world it is something like:

  - abstract object types are always off_t

  - lower-level code that takes an object in a buffer always uses size_t

  - at the conversion point, we must always use xsize_t() to catch
conversion problems.

  - Any time that xsize_t() might die is a place where the caller should
probably figure out if it can use a streaming code path for large
objects.

> This conversion would be a significant patch, so I wanted to get the
> community's thoughts on this conversion.
> 
> If there are small, isolated chunks that can be done safely, then this may
> be a good target for a first patch.

Yes, I agree this probably has to be done incrementally. The threads
Thomas linked might be good starting points. But do be careful with
incremental changes that you don't introduce any spots where we might
get a value mismatch that used to be consistently truncated. E.g., this
code is wrong but not a security problem:

  size_t full_len = strlen(src); /* ok */
  unsigned long len = full_len; /* possible truncation! */
  char *dst = xmalloc(len); /* buffer possibly smaller than full_len */
  memcpy(dst, src, len); /* copies truncated value */

But this has a vulnerability:

  memcpy(dst, src, full_len); /* whoops, buffer is too small! */

Obviously this is a stupid toy example, but in the real world things
like that "unsigned long" assignment happen via implicit conversion in
function parameters. Or we never have "full_len" in the first place, and
instead build it up over a series of operations that write into the
buffer. Etc.

I made a pass over the whole code-base a while back to fix any existing
problems and introduce helpers to make it harder to get this wrong
(e.g., by making sure a consistent value is used for allocating and
populating a buffer). So I _hope_ that it should be hard to accidentally
regress any code by switching out types. But it's something to be aware
of.

-Peff

Re: [PATCH v2] send-email: extract email-parsing code into a subroutine

[Junio C Hamano]

Nathan Payre  writes:

> The existing code mixes parsing of email header with regular
> expression and actual code. Extract the parsing code into a new
> subroutine 'parse_header_line()'. This improves the code readability
> and make parse_header_line reusable in other place.
>
> Signed-off-by: Nathan Payre 
> Signed-off-by: Matthieu Moy 
> Signed-off-by: Timothee Albertin 
> Signed-off-by: Daniel Bensoussan 
> Thanks-to: Ævar Arnfjörð Bjarmason 

Thanks, but... 

> +my %parsed_email;
> + $parsed_email{'body'} = '';
> +while (my $line = <$c>) {
> + next if $line =~ m/^GIT:/;
> + parse_header_line($line, \%parsed_email);
> + if ($line =~ /^\n$/i) {
> + while (my $body_line = <$c>) {
> +if ($body_line !~ m/^GIT:/) {
> +$parsed_email{'body'} = $parsed_email{'body'} . 
> $body_line;
> +}
> + }
> + }
> + print "la : $line\n";
> + }

... throughout this patch, not limited to this section, indentation
is strange and there seem to be many "print" that show messages that
do not seem to be meant for end-user consumption.  I can see that
this aspires to improve the readability, but not quite yet ;-).

Also "reusable in other place" is by itself not an unconditional
plus, until readers can be convinced that that 'other place' really
wants to be able to call this function.  Is there some untold
motivation behind this change---such as a planned update to actually
use this helper subroutine?

Re: [PATCH v2] pathspec: only match across submodule boundaries when requested

[Johannes Schindelin]

Hi Brandon,

On Mon, 4 Dec 2017, Brandon Williams wrote:

> Commit 74ed43711fd (grep: enable recurse-submodules to work on 
> objects, 2016-12-16) taught 'tree_entry_interesting()' to be able to
> match across submodule boundaries in the presence of wildcards.  This is
> done by performing literal matching up to the first wildcard and then
> punting to the submodule itself to perform more accurate pattern
> matching.  Instead of introducing a new flag to request this behavior,
> commit 74ed43711fd overloaded the already existing 'recursive' flag in
> 'struct pathspec' to request this behavior.
> 
> This leads to a bug where whenever any other caller has the 'recursive'
> flag set as well as a pathspec with wildcards that all submodules will
> be indicated as matches.  One simple example of this is:
> 
>   git init repo
>   cd repo
> 
>   git init submodule
>   git -C submodule commit -m initial --allow-empty
> 
>   touch "[bracket]"
>   git add "[bracket]"
>   git commit -m bracket
>   git add submodule
>   git commit -m submodule
> 
>   git rev-list HEAD -- "[bracket]"
> 
> Fix this by introducing the new flag 'recurse_submodules' in 'struct
> pathspec' and using this flag to determine if matches should be allowed
> to cross submodule boundaries.
> 
> This fixes https://github.com/git-for-windows/git/issues/1371.

Thanks,
Dscho

Re: partial_clone_get_default_filter_spec has no callers

[Jeff Hostetler]

On 12/6/2017 12:39 PM, Ramsay Jones wrote:

Hi Jeff,

commit f1862e8153 ("partial-clone: define partial clone settings
in config", 2017-12-05), which is part of your 'jh/partial-clone'
branch, introduces the partial_clone_get_default_filter_spec()
function without any callers. Could you please confirm that this
is intentional and that, presumably, a future series will include
a call to this function.


I'll double check.  Thanks.

BTW is there another tool that you're using to find these?
I know I ran make DEVELOPER=1 and make sparse on everything
and didn't see that come up.

Jeff

'git worktree add' does not fire post-checkout hook

[Gumbel, Matthew K]

Hello all,

I've noticed that when I run 'git worktree add /path/to/new/tree', the 
post-checkout hook does not fire, even though the worktree manpage explicitly 
states that "worktree add" will, "Create  and checkout  into it."

Is this the intended behavior? Seems like maybe a bug, but I'm not sure.

Thanks
Matt

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 6, 2017 at 7:56 PM, Daniel Jacques  wrote:
> On Wed, Dec 6, 2017 at 1:47 PM, Junio C Hamano  wrote:
>>
>> Johannes Sixt  writes:
>>
>> > The updated series works for me now. Nevertheless, I suggest to squash
>> > in the following change to protect against IFS and globbing characters in
>> > $INSTLIBDIR.
>>
>> Yeah, that is very sensible.
>>
>> > diff --git a/Makefile b/Makefile
>> > index 7ac4458f11..08c78a1a63 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) 
>> > GIT-PERL-DEFINES perl/perl.mak Makefile
>> >   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
>> >   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" 
>> > && \
>> >   sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
>> > - -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
>> > + -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
>> >   -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
>> >   -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
>> >   $< >$@+ && \
>
> Sounds good; I'll apply that to my working patch and include it in my
> next ("v5") submission, which is currently blocked pending avarab@'s Perl
> Makefile changes:
> https://public-inbox.org/git/20171129195430.10069-1-ava...@gmail.com/T/#t

Thanks, FWIW I'll send another version of that at the end of the week
or so, I'm waiting to see if there's any more comments on it to reduce
list churn.

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

[Daniel Jacques]

On Wed, Dec 6, 2017 at 1:47 PM, Junio C Hamano  wrote:
>
> Johannes Sixt  writes:
>
> > The updated series works for me now. Nevertheless, I suggest to squash
> > in the following change to protect against IFS and globbing characters in
> > $INSTLIBDIR.
>
> Yeah, that is very sensible.
>
> > diff --git a/Makefile b/Makefile
> > index 7ac4458f11..08c78a1a63 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) 
> > GIT-PERL-DEFINES perl/perl.mak Makefile
> >   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
> >   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && 
> > \
> >   sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
> > - -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
> > + -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
> >   -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
> >   -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
> >   $< >$@+ && \

Sounds good; I'll apply that to my working patch and include it in my
next ("v5") submission, which is currently blocked pending avarab@'s Perl
Makefile changes:
https://public-inbox.org/git/20171129195430.10069-1-ava...@gmail.com/T/#t

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

[Junio C Hamano]

Johannes Sixt  writes:

> The updated series works for me now. Nevertheless, I suggest to squash
> in the following change to protect against IFS and globbing characters in
> $INSTLIBDIR.

Yeah, that is very sensible.

> diff --git a/Makefile b/Makefile
> index 7ac4458f11..08c78a1a63 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) 
> GIT-PERL-DEFINES perl/perl.mak Makefile
>   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
>   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
>   sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
> - -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
> + -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
>   -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
>   -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
>   $< >$@+ && \

Re: [SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Junio C Hamano]

On Wed, Dec 6, 2017 at 10:34 AM, Johannes Sixt  wrote:
> I am sorry for not responding in detail. I think we've reached a mutual
> understanding of our workflows.
>
> Though, from the ideas you tossed around most recently, you seem to want to
> make git-commit into a kitchen-sink for everything. I have my doubts that
> this will be a welcome change. Just because new commits are created does not
> mean that the feature must live in git-commit.

Nicely put.

Re: [SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Johannes Sixt]

I am sorry for not responding in detail. I think we've reached a mutual 
understanding of our workflows.


Though, from the ideas you tossed around most recently, you seem to want 
to make git-commit into a kitchen-sink for everything. I have my doubts 
that this will be a welcome change. Just because new commits are created 
does not mean that the feature must live in git-commit.


-- Hannes

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

[Johannes Sixt]

Am 05.12.2017 um 22:35 schrieb Junio C Hamano:
> Dan Jacques  writes:
> 
>> Thanks for checking! The patch that you quoted above looks like it's from
>> this "v4" thread; however, the patch that you are diffing against in your
>> latest reply seems like it is from an earlier version.
>>
>> I believe that the $(pathsep) changes in your proposed patch are already
>> present in v4,...
> 
> You're of course right.  The patches I had in my tree are outdated.
> 
> Will replace, even though I won't be merging them to 'pu' while we
> wait for Ævar's perl build procedure update to stabilize.

The updated series works for me now. Nevertheless, I suggest to squash
in the following change to protect against IFS and globbing characters in
$INSTLIBDIR.

diff --git a/Makefile b/Makefile
index 7ac4458f11..08c78a1a63 100644
--- a/Makefile
+++ b/Makefile
@@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES 
perl/perl.mak Makefile
INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-   -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+   -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
-e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
-e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
$< >$@+ && \

Re: [RFC] 'unsigned long' to 'size_t' conversion

[Brandon Williams]

On 12/06, Derrick Stolee wrote:
> There are several places in Git where we refer to the size of an
> object by an 'unsigned long' instead of a 'size_t'. In 64-bit Linux,
> 'unsigned long' is 8 bytes, but in 64-bit Windows it is 4 bytes.
> 
> The main issue with this conversion is that large objects fail to
> load (they seem to hash and store just fine). For example, the
> following 'blob8gb' is an 8 GB file where the ith byte is equal to i
> % 256:
> 
> $ git hash-object -w --no-filters blob8gb
> 5391939346b98600acc0283dda24649450cec51f
> 
> $ git cat-file -s 5391939346b98600acc0283dda24649450cec51f
> error: bad object header
> fatal: git cat-file: could not get object info
> 
> An existing discussion can be found here:
> https://github.com/git-for-windows/git/issues/1063
> 
> The error message results from unpack_object_header_buffer() which
> had its most-recent meaningful change in 'ea4f9685:
> unpack_object_header_buffer(): clear the size field upon error' (in
> 2011).
> 
> In my opinion, the correct thing to do would be to replace all
> 'unsigned long's that refer to an object size and replace them with
> 'size_t'. However, a simple "git grep 'unsigned long size'" reveals
> 194 results, and there are other permutations of names and pointer
> types all over.
> 
> This conversion would be a significant patch, so I wanted to get the
> community's thoughts on this conversion.
> 
> If there are small, isolated chunks that can be done safely, then
> this may be a good target for a first patch.

I think that an effort like this would definitely be worthwhile.  Much
like the unsigned char[20] -> struct object_id conversion I would think
that the best way to go about such a conversion would be to do it in
small chunks as you've mentioned.  That way you are only causing churn
in hopefully small parts of the code base at a time instead of one
monolithic change that is sure to cause conflicts.

-- 
Brandon Williams

Re: git blame --reverse doesn't find line in HEAD

[Nick Snyder]

> Can you bisect to see when the feature stopped working as you expect?

I will see if I can do that but might take some time.

> It finds up to which commit each line survived without getting touched since 
> the oldest commit in the range.

Right, this is where it is failing in my case.

With a history like this:
A <- B <- C <- HEAD

I have a particular line in C (HEAD) that blames to commit A.
If I run a git blame --reverse starting at commit A for that line, it
doesn't give me back C, it gives me back B instead.
The line is not added/deleted/moved between B and C.



On Wed, Dec 6, 2017 at 9:22 AM, Junio C Hamano  wrote:
> Nick Snyder  writes:
>
>> This can be reproduced on Linux and Mac. This behavior seems to be a bug.
>
> Can you bisect to see when the feature stopped working as you expect?
>
> Unlike a forward blame, where the command tries to find a commit
> that is responsible for a line being in the final result (i.e.
> typically, HEAD), a reverse blame is not about finding a commit
> that is responsible for a line (that used to be in the oldest
> commit) not being in a more recent codebase.  It finds up to which
> commit each line survived without getting touched since the oldest
> commit in the range.
>

Re: [PATCH v3 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

On Sunday 03 December 2017 07:38 AM, Junio C Hamano wrote:

Kaartic Sivaraam  writes:



NOTE: Though a commit-hash is a "syntactically" valid branch name,
it is generally not considered as one for the use cases of
"git check-ref-format --branch". That's because a user who does
"git check-ref-format --branch @{-$N}" would except the output
to be a "existing" branch name apart from it being syntactically
valid.


s/except/expect/ I suspect.


Correct suspicion.



 But I do not think this description is
correct.  "check-ref-format --branch @{-1}", when you come from the
detached HEAD state, happily report success so it does not hold that
it is "generally not considered".

Unless you are saying "check-ref-format --branch" is buggy, that is.


I was thinking it was "buggy" from v1 of this patch. The `--branch` 
option is expected to be used by porcelains but they are also wanted to 
be aware that the output might NOT be a branch name when the @{-N} 
syntax is used[1]. This sounds unintuitive given the name of the 
option(`--branch`). No user would expect anything but a branch name from 
such an option, I guess. At least, I don't. So, I thought clarifying the 
Doc was a good "first step" (the Doc guaranteed more than it should).




If so, we shouldn't be casting that buggy behaviour in stone by
documenting, but should be fixing it.



Yes. I wasn't sure how to go about fixing it and thus took the easier 
way of updating the Doc. I also think it would be a good way to trigger 
discussion. Now that the attention has come, any ideas about how it 
could be FIXED? Should we drop support for @{-N} option in 
check-branch-ref (highly backward incompatible)? Should we check if 
@{-$N} is a branch name and fail if it's not(not such a bad thing, I guess)?




And the paragraph that leads to this NOTE and this NOTE itself are
both misleading from that point of view.  The result *is* always a
valid branch name,
I wasn't referring to "syntactic validity" when I mentioned "valid" in 
the commit message. Though, I understand how I wasn't clear by not 
disambiguating.





Taking the above together, perhaps.

 When the N-th previous thing checked out syntax (@{-N}) is used
 with '--branch' option of check-ref-format the result may not be
 the name of a branch that currently exists or ever existed.
 This is because @{-N} is used to refer to the N-th last checked
 out "thing", which might be any commit (sometimes a branch) in
 the detached HEAD state.


I don't get the "... any in the detached HEAD state ..." part. I seem to 
interpret it as "@{-N}" always returns commits from the detached HEAD 
state. How about,


When the N-th previous thing checked out syntax (@{-N}) is used
with '--branch' option of check-ref-format the result may not be
the name of a branch that currently exists or ever existed. This
is because @{-N} is used to refer to the N-th last checked out
"thing", which might be a commit object name if the previous check
out was a detached HEAD state; or a branch name, otherwise. The
documentation thus does a wrong thing by promoting it as the
"previous branch syntax".




 State that @{-N} is the syntax for specifying "N-th last thing
 checked out" and also state that the result of using @{-N} might
 also result in an commit object name.



This one's nice.



diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index cf0a0b7df..5ddb562d0 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,17 +78,20 @@ reference name expressions (see linkgit:gitrevisions[7]):
  . at-open-brace `@{` is used as a notation to access a reflog entry.
  
  With the `--branch` option, the command takes a name and checks if

-it can be used as a valid branch name (e.g. when creating a new
-branch).  The rule `git check-ref-format --branch $name` implements
+it can be used as a valid branch name e.g. when creating a new branch
+(except for one exception related to the previous checkout syntax
+noted below). The rule `git check-ref-format --branch $name` implements


And "except for" is also wrong here.  40-hex still *IS* a valid
branch name; it is just it may not be what we expect.  So perhaps
rephrase it to something like "(but be cautious when using the
previous checkout syntax that may refer to a detached HEAD state)".



Nice suggestion.



+`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
+was checkout using "git checkout" operation. This option should be


s/was checkout/was checked out/;



Good catch.



+used by porcelains to accept this syntax anywhere a branch name is
+expected, so they can act as if you typed the branch name. As an
+exception note that, the ``previous checkout operation'' might result
+in a commit hash when the N-th last thing checked out was not a branch.


s/a commit hash/a commit object name/;



Ok.

partial_clone_get_default_filter_spec has no callers

[Ramsay Jones]

Hi Jeff,

commit f1862e8153 ("partial-clone: define partial clone settings
in config", 2017-12-05), which is part of your 'jh/partial-clone'
branch, introduces the partial_clone_get_default_filter_spec()
function without any callers. Could you please confirm that this
is intentional and that, presumably, a future series will include
a call to this function.

Thanks!

ATB,
Ramsay Jones

Re: git blame --reverse doesn't find line in HEAD

[Junio C Hamano]

Nick Snyder  writes:

> This can be reproduced on Linux and Mac. This behavior seems to be a bug.

Can you bisect to see when the feature stopped working as you expect?

Unlike a forward blame, where the command tries to find a commit
that is responsible for a line being in the final result (i.e.
typically, HEAD), a reverse blame is not about finding a commit
that is responsible for a line (that used to be in the oldest
commit) not being in a more recent codebase.  It finds up to which
commit each line survived without getting touched since the oldest
commit in the range.

Re: [RFC] 'unsigned long' to 'size_t' conversion

[Thomas Braun]

Am 06.12.2017 um 16:08 schrieb Derrick Stolee:

Hi Derrick,

> If there are small, isolated chunks that can be done safely, then this
> may be a good target for a first patch.

Here are some pointers to past discussions:
-
https://public-inbox.org/git/trinity-9f703269-6f73-4f6d-b90b-45e09e1c094c-1489582854278@3capp-gmx-bs66/
-
https://public-inbox.org/git/1502527643-21944-1-git-send-email-martin@mail.zuhause/

I'm posting this mainly so that we can avoid, if possible, duplicated work.

Hope that helps,
Thomas

RE: [RFE] install-doc-quick.sh should accept a commit-ish

[Randall S. Becker]

On December 6, 2017 11:40 AM, Junio C Hamano wrote:
>"Randall S. Becker"  writes:
>> Having the git-manpages repo available is fantastic for platforms that 
>> cannot easily build documentation on demand, for example, when too 
>> many dependencies that do not build properly.
>> It would be really nice to have a version of install-doc-quick.sh to
either:
>> 1. Use whatever version is checked out in git-manpages; or
>> 2. Use the proper commit associated with the git commit being 
>> installed (0a8e923 for v2.6.0 , as an example); or
>> 3. Allow the commit to be passed through the Documentation Makefile on
demand so that any version of documentation can be installed.

>Do you mean something like this so that you can say "not the tip of the
master branch but this one?"

> Documentation/install-doc-quick.sh | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

>diff --git a/Documentation/install-doc-quick.sh
b/Documentation/install-doc-quick.sh
>index 327f69bcf5..83764f7537 100755
>--- a/Documentation/install-doc-quick.sh
>+++ b/Documentation/install-doc-quick.sh
>@@ -3,8 +3,9 @@
 
> repository=${1?repository}
> destdir=${2?destination}
>+head=${3+master}
>+GIT_DIR=
 
>-head=master GIT_DIR=
> for d in "$repository/.git" "$repository"
> do
>   if GIT_DIR="$d" git rev-parse refs/heads/master >/dev/null 2>&1

Providing I can pass that through make via something like quick-install-man
head=commit-ish, that's what I'm hoping.

Cheers,
Randall

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Junio C Hamano]

Torsten Bögershausen  writes:

>> Looks like t0027-auto-crlf.sh is failing on Windows:
>> https://travis-ci.org/git/git/jobs/312135514
>> 
>> Could that patch be related?
>> 
>> - Lars
>> 
> Chances are high, I will have a look.
> Thanks for noticing.

Thanks.

Re: [PATCH v7 4/7] checkout: describe_detached_head: remove ellipsis after committish

[Junio C Hamano]

Ann T Ropea  writes:

> v7: fix style issues (redirection, here-dox, long lines, 
> setting/exporting/unsetting of env var): cf. 
> 

Thanks.  I'd say this is polished enough already.  Let's stop
rerolling, wait for a few days for others to give final comments and
merge it to 'next'.

I've however queued the following on top of the entire series.  I do
not mind squashing it into this step, though.

-- >8 --
Subject: [PATCH] t2020: test variations that matter

Because our test suite is not about validating the working of the
shell, it is pointless to test variations of how a literal string
'yes' is quoted when assigned to an environment variable.

Instead, test various ways to spell 'yes' (we use strcasecmp() so
uppercased and capitalized variant should work just like 'yes'
spelled in all lowercase) and make sure we take them as 'yes'.  That
is more relevant in testing Git.

Signed-off-by: Junio C Hamano 
---
 t/t2020-checkout-detach.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 9464069d15..bb4f2e0c63 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -294,15 +294,15 @@ test_expect_success 'describe_detached_head does print 
SHA-1 ellipsis when asked
# Various ways of asking for ellipses...
# The user can just use any kind of quoting (including none).
 
-   GIT_PRINT_SHA1_ELLIPSIS="yes" git -c 'core.abbrev=12' checkout HEAD^ 
>actual 2>&1 &&
+   GIT_PRINT_SHA1_ELLIPSIS=yes git -c 'core.abbrev=12' checkout HEAD^ 
>actual 2>&1 &&
check_detached &&
test_i18ncmp 1st_detach actual &&
 
-   GIT_PRINT_SHA1_ELLIPSIS='yes' git -c 'core.abbrev=12' checkout HEAD^ 
>actual 2>&1 &&
+   GIT_PRINT_SHA1_ELLIPSIS=Yes git -c 'core.abbrev=12' checkout HEAD^ 
>actual 2>&1 &&
check_detached &&
test_i18ncmp 2nd_detach actual &&
 
-   GIT_PRINT_SHA1_ELLIPSIS=yes git -c 'core.abbrev=12' checkout HEAD^ 
>actual 2>&1 &&
+   GIT_PRINT_SHA1_ELLIPSIS=YES git -c 'core.abbrev=12' checkout HEAD^ 
>actual 2>&1 &&
check_detached &&
test_i18ncmp 3rd_detach actual &&
 
-- 
2.15.1-465-g070199a3f5

Re: [RFE] install-doc-quick.sh should accept a commit-ish

[Junio C Hamano]

"Randall S. Becker"  writes:

> Having the git-manpages repo available is fantastic for platforms
> that cannot easily build documentation on demand, for example,
> when too many dependencies that do not build properly.
>
> It would be really nice to have a version of install-doc-quick.sh to either:
>
> 1. Use whatever version is checked out in git-manpages; or
>
> 2. Use the proper commit associated with the git commit being installed 
> (0a8e923 for v2.6.0 , as an example); or
>
> 3. Allow the commit to be passed through the Documentation Makefile on demand 
> so that any version of documentation can be installed.

Do you mean something like this so that you can say "not the tip of
the master branch but this one?"

 Documentation/install-doc-quick.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/install-doc-quick.sh 
b/Documentation/install-doc-quick.sh
index 327f69bcf5..83764f7537 100755
--- a/Documentation/install-doc-quick.sh
+++ b/Documentation/install-doc-quick.sh
@@ -3,8 +3,9 @@
 
 repository=${1?repository}
 destdir=${2?destination}
+head=${3+master}
+GIT_DIR=
 
-head=master GIT_DIR=
 for d in "$repository/.git" "$repository"
 do
if GIT_DIR="$d" git rev-parse refs/heads/master >/dev/null 2>&1

[PATCH v2] send-email: extract email-parsing code into a subroutine

[Nathan Payre]

The existing code mixes parsing of email header with regular
expression and actual code. Extract the parsing code into a new
subroutine 'parse_header_line()'. This improves the code readability
and make parse_header_line reusable in other place.

Signed-off-by: Nathan Payre 
Signed-off-by: Matthieu Moy 
Signed-off-by: Timothee Albertin 
Signed-off-by: Daniel Bensoussan 
Thanks-to: Ævar Arnfjörð Bjarmason 
---
 git-send-email.perl | 100 ++--
 1 file changed, 73 insertions(+), 27 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2208dcc21..db16e4dec 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -715,41 +715,73 @@ EOT3
if (!defined $compose_encoding) {
$compose_encoding = "UTF-8";
}
-   while(<$c>) {
-   next if m/^GIT:/;
-   if ($in_body) {
-   $summary_empty = 0 unless (/^\n$/);
-   } elsif (/^\n$/) {
-   $in_body = 1;
-   if ($need_8bit_cte) {
+
+my %parsed_email;
+   $parsed_email{'body'} = '';
+while (my $line = <$c>) {
+   next if $line =~ m/^GIT:/;
+   parse_header_line($line, \%parsed_email);
+   if ($line =~ /^\n$/i) {
+   while (my $body_line = <$c>) {
+if ($body_line !~ m/^GIT:/) {
+$parsed_email{'body'} = $parsed_email{'body'} . $body_line;
+}
+   }
+   }
+   print "la : $line\n";
+   }
+
+   if ($parsed_email{'from'}) {
+   $sender = $parsed_email{'from'};
+   }
+   if ($parsed_email{'in-reply-to'}) {
+   $initial_reply_to = $parsed_email{'in-reply-to'};
+   }
+   if ($parsed_email{'subject'}) {
+   $initial_subject = $parsed_email{'subject'};
+   print $c2 "Subject: " .
+   quote_subject($parsed_email{'subject'}, 
$compose_encoding) .
+   "\n";
+   }
+   if ($parsed_email{'mime-version'}) {
+   print "CASE 0\n";
+   $need_8bit_cte = 0;
+   print $c2 "MIME-Version: $parsed_email{'mime-version'}\n",
+   "Content-Type: 
$parsed_email{'content-type'};\n",
+   "Content-Transfer-Encoding: 
$parsed_email{'content-transfer-encoding'}\n";
+   }
+   if ($need_8bit_cte) {
+   if ($parsed_email{'content-type'}) {
+   print "CASE 1\n";
+   print $c2 "MIME-Version: 1.0\n",
+"Content-Type: 
$parsed_email{'content-type'};",
+"Content-Transfer-Encoding: 8bit\n";
+   } else {
+   print "CASE 2\n";
print $c2 "MIME-Version: 1.0\n",
 "Content-Type: text/plain; ",
-  "charset=$compose_encoding\n",
+"charset=$compose_encoding\n",
 "Content-Transfer-Encoding: 8bit\n";
}
-   } elsif (/^MIME-Version:/i) {
-   $need_8bit_cte = 0;
-   } elsif (/^Subject:\s*(.+)\s*$/i) {
-   $initial_subject = $1;
-   my $subject = $initial_subject;
-   $_ = "Subject: " .
-   quote_subject($subject, $compose_encoding) .
-   "\n";
-   } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
-   $initial_reply_to = $1;
-   next;
-   } elsif (/^From:\s*(.+)\s*$/i) {
-   $sender = $1;
-   next;
-   } elsif (/^(?:To|Cc|Bcc):/i) {
-   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
-   next;
-   }
-   print $c2 $_;
}
+   if ($parsed_email{'body'}) {
+   $summary_empty = 0;
+   print $c2 "\n$parsed_email{'body'}\n";
+   }
+
close $c;
close $c2;
 
+   open $c2, "<", $compose_filename . ".final"
+   or die sprintf(__("Failed to open %s.final: %s"), 
$compose_filename, $!);
+
+   print "affichage : \n";
+   while (<$c2>) {
+   print $_;
+   }
+
+   close $c2;
+
if ($summary_empty) {
print __("Summary email is empty, skipping it\n");
$compose = -1;
@@ -792,6 +824,20 @@ sub ask {
return;
 }
 
+sub parse_header_line {

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Torsten Bögershausen]

On 2017-12-06 16:14, Lars Schneider wrote:
> 
>> On 04 Dec 2017, at 22:46, Junio C Hamano  wrote:
>>
>>
>> * tb/check-crlf-for-safe-crlf (2017-11-27) 1 commit
>> - convert: tighten the safe autocrlf handling
>>
>> The "safe crlf" check incorrectly triggered for contents that does
>> not use CRLF as line endings, which has been corrected.
>>
>> Will merge to 'next'.
>>
> 
> Looks like t0027-auto-crlf.sh is failing on Windows:
> https://travis-ci.org/git/git/jobs/312135514
> 
> Could that patch be related?
> 
> - Lars
> 
Chances are high, I will have a look.
Thanks for noticing.

[RFE] install-doc-quick.sh should accept a commit-ish

[Randall S. Becker]

Other thread attached as context.

Having the git-manpages repo available is fantastic for platforms that cannot 
easily build documentation on demand, for example, when too many dependencies 
that do not build properly. 

It would be really nice to have a version of install-doc-quick.sh to either:

1. Use whatever version is checked out in git-manpages; or

2. Use the proper commit associated with the git commit being installed 
(0a8e923 for v2.6.0 , as an example); or

3. Allow the commit to be passed through the Documentation Makefile on demand 
so that any version of documentation can be installed.

Thanks,
Randall
P.S. If the idea is liked, I can try to make this happen.

-Original Message-
From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf Of 
Randall S. Becker
Sent: December 6, 2017 10:43 AM
To: 'Jeff King' ; 'Ævar Arnfjörð Bjarmason' ; 
'Junio C Hamano' 
Cc: git@vger.kernel.org
Subject: RE: Documentation Breakage at 2.5.6

-Original Message-
On December 6, 2017 3:49 AM, Jeff King wrote:
>On Wed, Dec 06, 2017 at 09:14:57AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> > I'm trying to upgrade the NonStop port from 2.3.7 upward eventually 
>> > to
>> > 2.15.1 and hit a snag on documentation. The xmlto component is a 
>> > bit new to me and I hit the following error:
>Did it work before in v2.3.7? If so, can you bisect to the breakage?
It worked fine at 2.3.7. No seeming dependency on docbook at that point - it 
was never on my system.

>One alternative is to try to avoid docbook entirely. The only way to get 
>manpages with asciidoc is to generate docbook and then process it, but:
I have asciidoc installed, but using it via Make?

> - you can generate HTML directly (and "make -C Documentation html" 
> does  this). Perhaps not as nice, but you still at least have some
>   documentation.
Not an option. I need git help to work.

> - asciidoctor can generate manpages directly. I don't think our
>   Makefile supports that now, but it might not be too hard to hack in  
> (we already have some basic asciidoctor support). I'm not sure how
 > hard it would be to get Ruby running on NonStop Ruby runs fine. I'm a bit 
 > out of my configuration depth here.

>And of course one final option is to generate the manpages elsewhere and copy 
>them in, since they're platform-independent.
>In fact, that's what quick-install-man should do (you just have to clone 
>Junio's >git-manpages repository -- see the INSTALL file).

I've gone down this path and it works. Much cleaner in fact. Dependencies of 
docbook (jade) are too reliant on GCC C++ forms to port to the platform - not 
to mention being SVN, which is culturally uncomfortable 

One request to Junio: Would it be possible to tag the commits to align with the 
tags in the main repo? That way, I can build a nice little Jenkins job to 
automatically fetch the correct commit for man pages when packaging up a 
release.

-Peff

RE: Documentation Breakage at 2.5.6

[Randall S. Becker]

-Original Message-
On December 6, 2017 3:49 AM, Jeff King wrote:
>On Wed, Dec 06, 2017 at 09:14:57AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> > I'm trying to upgrade the NonStop port from 2.3.7 upward eventually 
>> > to
>> > 2.15.1 and hit a snag on documentation. The xmlto component is a bit 
>> > new to me and I hit the following error:
>Did it work before in v2.3.7? If so, can you bisect to the breakage?
It worked fine at 2.3.7. No seeming dependency on docbook at that point - it 
was never on my system.

>One alternative is to try to avoid docbook entirely. The only way to get 
>manpages with asciidoc is to generate docbook and then process it, but:
I have asciidoc installed, but using it via Make?

> - you can generate HTML directly (and "make -C Documentation html" does
>  this). Perhaps not as nice, but you still at least have some
>   documentation.
Not an option. I need git help to work.

> - asciidoctor can generate manpages directly. I don't think our
>   Makefile supports that now, but it might not be too hard to hack in
>  (we already have some basic asciidoctor support). I'm not sure how
 > hard it would be to get Ruby running on NonStop
Ruby runs fine. I'm a bit out of my configuration depth here.

>And of course one final option is to generate the manpages elsewhere and copy 
>them in, since they're platform-independent.
>In fact, that's what quick-install-man should do (you just have to clone 
>Junio's >git-manpages repository -- see the INSTALL file).

I've gone down this path and it works. Much cleaner in fact. Dependencies of 
docbook (jade) are too reliant on GCC C++ forms to port to the platform - not 
to mention being SVN, which is culturally uncomfortable 

One request to Junio: Would it be possible to tag the commits to align with the 
tags in the main repo? That way, I can build a nice little Jenkins job to 
automatically fetch the correct commit for man pages when packaging up a 
release.

-Peff

Re: git blame --reverse doesn't find line in HEAD

[Nick Snyder]

This can be reproduced on Linux and Mac. This behavior seems to be a bug.

On Wed, Nov 29, 2017 at 12:06 AM, Nick Snyder  wrote:
> I have a repo that reproduces a behavior with `git blame --reverse`
> that surprises me. https://github.com/nicksnyder/git-blame-bug
>
> The readme explains the observed behavior and what I expected to
> happen. I will inline the readme at the bottom of this message for
> convenience.
>
> Am I misunderstanding --reverse or is this a bug?
>
> Thanks!
> Nick
>
> $ git --version
> git version 2.15.0
>
> Blame of L465 in Tree.tsx at HEAD (ca0fb5) points to L463 at 199ee7
>
> $ git blame -p -L465,465 Tree.tsx
> 199ee75d1240ae72cd965f62aceeb301ab64e1bd 463 465 1
> filename Tree.tsx
> public shouldComponentUpdate(nextProps: TileProps): boolean {
>
> EXPECTED: Reverse blame of L463 at 199ee7 points to L465 at the
> lastest commit in the repo (at least ca0fb5).
> ACTUAL: Reverse blame of L463 at 199ee7 points to L463 at 199ee7.
>
> $ git blame -p -L463,463 --reverse 199ee7.. Tree.tsx
> 199ee75d1240ae72cd965f62aceeb301ab64e1bd 463 463 1
> boundary
> previous ca0fb5a2d61cb16909bcb06f49dd5448a26f32b1 Tree.tsx
> filename Tree.tsx
> public shouldComponentUpdate(nextProps: TileProps): boolean {
>
> The line in question is in the diff (git diff 199ee7..ca0fb5), but
> that particular line is neither added nor deleted, so I don't know why
> blame would think it is deleted.
>
> Relevant hunk in diff:
>
> @@ -452,28 +462,17 @@ export class LayerTile extends
> React.Component {
>  }
>  }
>
> -public validTokenRange(props: TileProps): boolean {
> -if (props.selectedPath === '') {
> -return true
> -}
> -const token = props.selectedPath.split('/').pop()!
> -return token >= this.first && token <= this.last
> -}
> -
>  public shouldComponentUpdate(nextProps: TileProps): boolean {
> -const lastValid = this.validTokenRange(this.props)
> -const nextValid = this.validTokenRange(nextProps)
> -if (!lastValid && !nextValid) {
> -// short circuit
> -return false
> +if (isEqualOrAncestor(this.props.selectedDir,
> this.props.currSubpath)) {
> +return true
>  }
> -if (isEqualOrAncestor(this.props.selectedDir,
> this.props.currSubpath) && lastValid) {
> +if (nextProps.selectedDir === nextProps.currSubpath) {
>  return true
>  }
> -if (nextProps.selectedDir === nextProps.currSubpath &&
> this.validTokenRange(nextProps)) {
> +if (getParentDir(nextProps.selectedDir) === nextProps.currSubpath) {
>  return true
>  }
> -if (getParentDir(nextProps.selectedDir) ===
> nextProps.currSubpath && this.validTokenRange(nextProps)) {
> +if (!isEqual(nextProps.pathSplits, this.props.pathSplits)) {
>  return true
>  }
>  return false

[PATCH v2] send-email: extract email-parsing code into a subroutine

[Nathan Payre]

The existing code mixes parsing of email header with regular
expression and actual code. Extract the parsing code into a new
subroutine 'parse_header_line()'. This improves the code readability
and make parse_header_line reusable in other place.

Signed-off-by: Nathan Payre 
Signed-off-by: Matthieu Moy 
Signed-off-by: Timothee Albertin 
Signed-off-by: Daniel Bensoussan 
Thanks-to: Ævar Arnfjörð Bjarmason 
---
 git-send-email.perl | 100 ++--
 1 file changed, 73 insertions(+), 27 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2208dcc21..db16e4dec 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -715,41 +715,73 @@ EOT3
if (!defined $compose_encoding) {
$compose_encoding = "UTF-8";
}
-   while(<$c>) {
-   next if m/^GIT:/;
-   if ($in_body) {
-   $summary_empty = 0 unless (/^\n$/);
-   } elsif (/^\n$/) {
-   $in_body = 1;
-   if ($need_8bit_cte) {
+
+my %parsed_email;
+   $parsed_email{'body'} = '';
+while (my $line = <$c>) {
+   next if $line =~ m/^GIT:/;
+   parse_header_line($line, \%parsed_email);
+   if ($line =~ /^\n$/i) {
+   while (my $body_line = <$c>) {
+if ($body_line !~ m/^GIT:/) {
+$parsed_email{'body'} = $parsed_email{'body'} . $body_line;
+}
+   }
+   }
+   print "la : $line\n";
+   }
+
+   if ($parsed_email{'from'}) {
+   $sender = $parsed_email{'from'};
+   }
+   if ($parsed_email{'in-reply-to'}) {
+   $initial_reply_to = $parsed_email{'in-reply-to'};
+   }
+   if ($parsed_email{'subject'}) {
+   $initial_subject = $parsed_email{'subject'};
+   print $c2 "Subject: " .
+   quote_subject($parsed_email{'subject'}, 
$compose_encoding) .
+   "\n";
+   }
+   if ($parsed_email{'mime-version'}) {
+   print "CASE 0\n";
+   $need_8bit_cte = 0;
+   print $c2 "MIME-Version: $parsed_email{'mime-version'}\n",
+   "Content-Type: 
$parsed_email{'content-type'};\n",
+   "Content-Transfer-Encoding: 
$parsed_email{'content-transfer-encoding'}\n";
+   }
+   if ($need_8bit_cte) {
+   if ($parsed_email{'content-type'}) {
+   print "CASE 1\n";
+   print $c2 "MIME-Version: 1.0\n",
+"Content-Type: 
$parsed_email{'content-type'};",
+"Content-Transfer-Encoding: 8bit\n";
+   } else {
+   print "CASE 2\n";
print $c2 "MIME-Version: 1.0\n",
 "Content-Type: text/plain; ",
-  "charset=$compose_encoding\n",
+"charset=$compose_encoding\n",
 "Content-Transfer-Encoding: 8bit\n";
}
-   } elsif (/^MIME-Version:/i) {
-   $need_8bit_cte = 0;
-   } elsif (/^Subject:\s*(.+)\s*$/i) {
-   $initial_subject = $1;
-   my $subject = $initial_subject;
-   $_ = "Subject: " .
-   quote_subject($subject, $compose_encoding) .
-   "\n";
-   } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
-   $initial_reply_to = $1;
-   next;
-   } elsif (/^From:\s*(.+)\s*$/i) {
-   $sender = $1;
-   next;
-   } elsif (/^(?:To|Cc|Bcc):/i) {
-   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
-   next;
-   }
-   print $c2 $_;
}
+   if ($parsed_email{'body'}) {
+   $summary_empty = 0;
+   print $c2 "\n$parsed_email{'body'}\n";
+   }
+
close $c;
close $c2;
 
+   open $c2, "<", $compose_filename . ".final"
+   or die sprintf(__("Failed to open %s.final: %s"), 
$compose_filename, $!);
+
+   print "affichage : \n";
+   while (<$c2>) {
+   print $_;
+   }
+
+   close $c2;
+
if ($summary_empty) {
print __("Summary email is empty, skipping it\n");
$compose = -1;
@@ -792,6 +824,20 @@ sub ask {
return;
 }
 
+sub parse_header_line {

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Lars Schneider]

> On 04 Dec 2017, at 22:46, Junio C Hamano  wrote:
> 
> 
> * tb/check-crlf-for-safe-crlf (2017-11-27) 1 commit
> - convert: tighten the safe autocrlf handling
> 
> The "safe crlf" check incorrectly triggered for contents that does
> not use CRLF as line endings, which has been corrected.
> 
> Will merge to 'next'.
> 

Looks like t0027-auto-crlf.sh is failing on Windows:
https://travis-ci.org/git/git/jobs/312135514

Could that patch be related?

- Lars

[RFC] 'unsigned long' to 'size_t' conversion

[Derrick Stolee]

There are several places in Git where we refer to the size of an object 
by an 'unsigned long' instead of a 'size_t'. In 64-bit Linux, 'unsigned 
long' is 8 bytes, but in 64-bit Windows it is 4 bytes.


The main issue with this conversion is that large objects fail to load 
(they seem to hash and store just fine). For example, the following 
'blob8gb' is an 8 GB file where the ith byte is equal to i % 256:


$ git hash-object -w --no-filters blob8gb
5391939346b98600acc0283dda24649450cec51f

$ git cat-file -s 5391939346b98600acc0283dda24649450cec51f
error: bad object header
fatal: git cat-file: could not get object info

An existing discussion can be found here: 
https://github.com/git-for-windows/git/issues/1063


The error message results from unpack_object_header_buffer() which had 
its most-recent meaningful change in 'ea4f9685: 
unpack_object_header_buffer(): clear the size field upon error' (in 2011).


In my opinion, the correct thing to do would be to replace all 'unsigned 
long's that refer to an object size and replace them with 'size_t'. 
However, a simple "git grep 'unsigned long size'" reveals 194 results, 
and there are other permutations of names and pointer types all over.


This conversion would be a significant patch, so I wanted to get the 
community's thoughts on this conversion.


If there are small, isolated chunks that can be done safely, then this 
may be a good target for a first patch.


Thanks,

-Stolee

Re: Git Repository Migration

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 6, 2017 at 3:02 PM, Saurabh Dixit  wrote:
> Hi,
>
> I am new here. I just wondered if the Merge Requests (aka., Pull
> Requests on GitHub) are also imported or cloned while
> cloning/importing a Git repository, say from GitHub to BitBucket.
> While I consider that, it may not be possible because of the URL to a
> remote is already set and cannot be altered while the Import/Clone ( I
> could be wrong at the assumption); I am curious to know what actually
> goes behind the scene.

For Github in particular you can check out the pull refs and push them
to your new hosting provider:
https://help.github.com/articles/checking-out-pull-requests-locally/

But that's just the refs, you won't get any of the discussion around
the pull requests or other "issues" etc.

Git Repository Migration

[Saurabh Dixit]

Hi,

I am new here. I just wondered if the Merge Requests (aka., Pull
Requests on GitHub) are also imported or cloned while
cloning/importing a Git repository, say from GitHub to BitBucket.
While I consider that, it may not be possible because of the URL to a
remote is already set and cannot be altered while the Import/Clone ( I
could be wrong at the assumption); I am curious to know what actually
goes behind the scene.

Best regards,

Saurabh Dixit
Opensource Enthusiast

Kindly Asists

[Mr. Sonami]

Kindly Assist Me


In good faith from Mr.Sonami, actually could you please consider to
help me to relocate this sum of five million,three hundred thousand
dollars(US$5.3 m) to your country for establishing a medium industry
in your country. The said 5.3 millions dollars was deposited in our
bank by Mr. Jean-Noël Rey a Swissland citizen who died in a  terrorist
attacks in 2016. We have never met before, but need not to worry as I
am using the only secured and confidential medium available to seek
for your foreign assistance in this business. I am contacting you
independently of my investigation and no one is informed of this
communication.

I contacted you in this transaction on my search for a reliable,
capable partner and  I discovered that the late Swiss died along side
with his supposed to be next of kin through CNN News on terrorist
attacks. I will give you all vital information concerning the Swiss
and the 5.3 millions dollars in our custody so that you will contact
my bank for them to release the money to you. You can come here in
person or you can request the bank to give you the contact of the bank
lawyer's who can represent your interest in the transfer process.


Reply and send this information to me at: sonami...@gmail.com

Your full Name.
Age...
Country
Occupation..
Your Telephone Numbers .

Have a nice day and good luck
Mr.Sonami

Assalamu`Alaikum.

[Mohammad Ouattara]

Greetings from Dr. mohammad ouattara.

Assalamu`Alaikum.

My Name is Dr. mohammad ouattara, I am a banker by profession. I'm
from Ouagadougou, Burkina Faso, West Africa. My reason for contacting
you is to transfer an abandoned $14.6M to your account.

The owner of this fund died since 2004 with his Next Of Kin. I want to
present you to the bank as the Next of Kin/beneficiary of this fund.

Further details of the transaction shall be forwarded to you as soon
as I receive your return mail indicating your interest.

1) YOUR FULL NAME...
(2) YOUR AGE AND SEX
(3) YOUR CONTACT ADDRESS..
(4) YOUR PRIVATE PHONE N0..
(5) FAX NUMBER..
(6) YOUR COUNTRY OF ORIGIN..
(7) YOUR OCCUPATION.

Have a great day,
Dr. mohammad ouattara.

Re: Documentation Breakage at 2.5.6

[Jeff King]

On Wed, Dec 06, 2017 at 09:14:57AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > I'm trying to upgrade the NonStop port from 2.3.7 upward eventually to
> > 2.15.1 and hit a snag on documentation. The xmlto component is a bit new to
> > me and I hit the following error:

Did it work before in v2.3.7? If so, can you bisect to the breakage?

> > XMLTO git-remote-testgit.1
> > xmlto: /home/git/git/Documentation/git-remote-testgit.xml does not validate
> > (status 3)
> > xmlto: Fix document syntax or use --skip-validation option
> > I/O error : Attempt to load network entity
> > http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd
> > /home/git/git/Documentation/git-remote-testgit.xml:2: warning: failed to
> > load external entity
> > "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd;
> > D DocBook XML V4.5//EN"
> > "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd;
> >
> > ^
> > I/O error : Attempt to load network entity
> > http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd
> > warning: failed to load external entity
> > "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd;
> > validity error : Could not load the external subset
> > http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd

Those URLs are the "official" names of the docbook DTDs. But normally
you'd have a local copy, along with a mapping from the official name to
your local copy. The XML term for that mapping is a "catalog", and it
looks something like this:

  $ grep oasis-open /etc/xml/catalog
  http://www.oasis-open.org/docbook/xml/; 
catalog="file:///etc/xml/docbook-xml.xml"/>

That just points to another local catalog, which has:

  $ grep 4.5/docbookx.dtd /etc/xml/docbook-xml.xml
  http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd; 
catalog="file:///usr/share/xml/docbook/schema/dtd/4.5/catalog.xml"/>
  http://docbook.org/xml/4.5/docbookx.dtd; 
catalog="file:///usr/share/xml/docbook/schema/dtd/4.5/catalog.xml"/>

So my guess is that your problem is one of:

  1. You don't have docbook 4.5 installed on your system.

or

  2. You don't have a correctly built catalog file, or xmlto isn't
 pointing to it for some reason (on Debian, this is normally built
 by the post-install script of packages that contain xml).

And xmlto (actually, probably xsltproc that it's calling) is unwilling
or unable to hit the network to pull down those entities.

Those are all somewhat vague guesses based on past troubles I've had
with broken xml setups. I'm far from an expert on xml processing (and
I'd just as soon keep it that way).

> I don't know if this helps, but here with xmlto 0.0.28 on Debian if I
> apply this the docs still build:
> 
> diff --git a/Documentation/texi.xsl b/Documentation/texi.xsl
> index 0f8ff07eca..332a65558d 100644
> --- a/Documentation/texi.xsl
> +++ b/Documentation/texi.xsl
> @@ -7,7 +7,7 @@
>   encoding="UTF-8"
> doctype-public="-//OASIS//DTD DocBook XML V4.5//EN"
> -   
> doctype-system="http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd; />
> +   
> doctype-system="http://example.org/docbook/xml/4.5/docbookx.dtd; />
> 
> So whatever's needing to remote fetch those resources doesn't seem to
> cause the same error for me.

I think that would come into play only if you try to build
"gitman.info", which isn't one of the default targets.

The string that Randall is seeing is in git-remote-testgit.xml, so it's
probably be generated by the "docbook" backend of asciidoc.

One alternative is to try to avoid docbook entirely. The only way to get
manpages with asciidoc is to generate docbook and then process it, but:

 - you can generate HTML directly (and "make -C Documentation html" does
   this). Perhaps not as nice, but you still at least have some
   documentation.

 - asciidoctor can generate manpages directly. I don't think our
   Makefile supports that now, but it might not be too hard to hack in
   (we already have some basic asciidoctor support). I'm not sure how
   hard it would be to get Ruby running on NonStop

And of course one final option is to generate the manpages elsewhere and
copy them in, since they're platform-independent. In fact, that's what
quick-install-man should do (you just have to clone Junio's git-manpages
repository -- see the INSTALL file).

-Peff

Re: Documentation Breakage at 2.5.6

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 06 2017, Randall S. Becker jotted:

> Hi All,
>
> I'm trying to upgrade the NonStop port from 2.3.7 upward eventually to
> 2.15.1 and hit a snag on documentation. The xmlto component is a bit new to
> me and I hit the following error:
>
> XMLTO git-remote-testgit.1
> xmlto: /home/git/git/Documentation/git-remote-testgit.xml does not validate
> (status 3)
> xmlto: Fix document syntax or use --skip-validation option
> I/O error : Attempt to load network entity
> http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd
> /home/git/git/Documentation/git-remote-testgit.xml:2: warning: failed to
> load external entity
> "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd;
> D DocBook XML V4.5//EN"
> "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd;
>
> ^
> I/O error : Attempt to load network entity
> http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd
> warning: failed to load external entity
> "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd;
> validity error : Could not load the external subset
> http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd
>
> The -skip-validation option just takes me to a different problem validating
> via sourceforge URL that appears not to exist anymore, although I had to
> modify ./git/Documention/Makefile, which is vexing.
>
> XMLTO git-remote-testgit.1
> I/O error : Attempt to load network entity
> http://docbook.sourceforge.net/release/xsl/current/manpages/docbook.xsl
> warning: failed to load external entity
> "http://docbook.sourceforge.net/release/xsl/current/manpages/docbook.xsl;
> compilation error: file /tmp/xmlto-xsl.ie6J8p line 4 element import
> xsl:import : unable to load
> http://docbook.sourceforge.net/release/xsl/current/manpages/docbook.xsl
> Makefile:328: recipe for target 'git-remote-testgit.1' failed
>
> Any advice on getting past this? It would be nice to get git help to working
> again. An answer of "you need to get past 2.5.6" is ok too as long as I know
> where I'm going.

I don't know if this helps, but here with xmlto 0.0.28 on Debian if I
apply this the docs still build:

diff --git a/Documentation/texi.xsl b/Documentation/texi.xsl
index 0f8ff07eca..332a65558d 100644
--- a/Documentation/texi.xsl
+++ b/Documentation/texi.xsl
@@ -7,7 +7,7 @@
 http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd; />
+   
doctype-system="http://example.org/docbook/xml/4.5/docbookx.dtd; />

So whatever's needing to remote fetch those resources doesn't seem to
cause the same error for me.


> -- Brief whoami: NonStop developer since approximately
> UNIX(421664400)/NonStop(2112884442)
> -- In my real life, I talk too much.