threads / rfc / 38019

[RFC 2/4] Properly accept quoted space in filenames

Subject: [RFC 2/4] Properly accept quoted space in filenames

## tl;dr

19 messages between Nov 20, 2014 and Nov 23, 2014.

replies: 18people: 3as markdown or json

Philip Oakley· Nov 20, 2014, 23:37 UTC · lore

[RFC 0/4] Fix the Visual Studio 2008 .sln generator

Potential Windows developers are likely to be using Visual Studio as their IDE. The tool stack required for Windows can be tortuous as it crosses the boundaries between platforms and philosophies. This RFC seeks to maintain the tools that could assist such developers. In particular, those tools that generate an initial Visual Studio project (.sln ) file.

The .sln generator in contrib began to break when internationalisation introduced an extra -o option. This recently worsened with the addition of invalidcontinue.obj for 'improved POSIX compatibility'.

I hacked a bit when I first attempted to use the VS IDE and noticed the i18n issue. I didn't completely solve all my issues because of further issues with VS2010 Express, so no patches were submitted at the time.

Now, with a fresh copy of VS20008 Express, I see the additional problem of the addition of the invalidcontinue.obj reference causing the .sln generation to fail.

Are the patches going in the right direction? Is the processing of the .obj file in engine.pl sensible? and the extra care with s/\.o$/.c/ avoiding s/obj/cbj/. Does it affect the Qmake capability? (I've no idea) Is the quoting of filenames correct? (my perl foo is cargo cult!)

I've also updated the vcbuild/README to mention Msysgit (which will be replaced soon by the newer/better Git-for-windows/SDK (https://github.com/git-for-windows/sdk), but the benefits still apply.

I've cc'd those who have contributed or patched the engine.pl, or appear to be interested via a $gmane search, who can hopefully comment.

Obviously, the patches will need squashing together, and the commit message(s) tidied after inclusion of comments.

Philip Oakley (4):
  Fix i18n -o option in msvc engine.pl
  Properly accept quoted space in filenames
  engine.pl: split the .o and .obj processing
  Improve layout and reference msvc-build script
 compat/vcbuild/README          | 29 +++++++++++++++++++++--------
 contrib/buildsystems/engine.pl | 38 ++++++++++++++++++++++++++++++--------
 2 files changed, 51 insertions(+), 16 deletions(-)
-- 
1.9.4.msysgit.0

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Philip Oakley· Nov 20, 2014, 23:37 UTC · re: Philip Oakley · lore

[RFC 1/4] Fix i18n -o option in msvc engine.pl

    The i18n 5e9637c6 introduced an extra '-o' option
    into the make file, which broke engine.pl code for
    extracting the git.sln for msvc gui-IDE.
    add tests for 'msgfmt' and its precursor 'mkdir'
    (in same vein as 74cf9bdda6).
    Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
 contrib/buildsystems/engine.pl | 12 ++++++++++++
 1 file changed, 12 insertions(+)
diff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl
index 23da787..9144ea7 100755
--- a/contrib/buildsystems/engine.pl
+++ b/contrib/buildsystems/engine.pl
@@ -140,6 +140,18 @@ sub parseMakeOutput
             next;
         }
 
+        if ($text =~ /^mkdir /) {
+            # options to the Portable Object translations in the line
+            # mkdir -p po/... && msgfmt ... (eg -o) may be mistaken for linker options
+            next;
+        }
+
+        if ($text =~ /^msgfmt /) {
+            # options to the Portable Object translations in the line
+            # mkdir -p po/... && msgfmt ... (eg -o) may be mistaken for linker options
+            next;
+        }
+
         if($text =~ / -c /) {
             # compilation
             handleCompileLine($text, $line);

-- 
1.9.4.msysgit.0

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Johannes Schindelin· Nov 21, 2014, 09:41 UTC · re: Philip Oakley · lore

Re: [RFC 1/4] Fix i18n -o option in msvc engine.pl

Hi Philip,
On Thu, 20 Nov 2014, Philip Oakley wrote:
>     The i18n 5e9637c6 introduced an extra '-o' option
>     into the make file,
I take it you are referring to
	https://github.com/git/git/commit/5e9637c6#diff-b67911656ef5d18c4ae36cb6741b7965R2195
Show 11 quoted lines
> diff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl
> index 23da787..9144ea7 100755
> --- a/contrib/buildsystems/engine.pl
> +++ b/contrib/buildsystems/engine.pl
> @@ -140,6 +140,18 @@ sub parseMakeOutput
>              next;
>          }
>  
> +        if ($text =~ /^mkdir /) {
> +            # options to the Portable Object translations in the line
> +            # mkdir -p po/... && msgfmt ... (eg -o) may be mistaken for linker options
Maybe better
		# the line "mkdir ... && msgfmt ..." contains no linker options
Show 8 quoted lines
> +            next;
> +        }
> +
> +        if ($text =~ /^msgfmt /) {
> +            # options to the Portable Object translations in the line
> +            # mkdir -p po/... && msgfmt ... (eg -o) may be mistaken for linker options
> +            next;
> +        }
These two if clauses do the same, maybe call it
	if ($test =~ /^(mkdir|msgfmt) /)

Ciao, Johannes

Philip Oakley· Nov 21, 2014, 19:57 UTC · re: Johannes Schindelin · lore

Re: [RFC 1/4] Fix i18n -o option in msvc engine.pl

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Show 10 quoted lines
> Hi Philip,
>
> On Thu, 20 Nov 2014, Philip Oakley wrote:
>
>>     The i18n 5e9637c6 introduced an extra '-o' option
>>     into the make file,
>
> I take it you are referring to
>
> https://github.com/git/git/commit/5e9637c6#diff-b67911656ef5d18c4ae36cb6741b7965R2195
Yes, Makefile#L2195 (of the new version)
Show 19 quoted lines
>
>> diff --git a/contrib/buildsystems/engine.pl 
>> b/contrib/buildsystems/engine.pl
>> index 23da787..9144ea7 100755
>> --- a/contrib/buildsystems/engine.pl
>> +++ b/contrib/buildsystems/engine.pl
>> @@ -140,6 +140,18 @@ sub parseMakeOutput
>>              next;
>>          }
>>
>> +        if ($text =~ /^mkdir /) {
>> +            # options to the Portable Object translations in the 
>> line
>> +            # mkdir -p po/... && msgfmt ... (eg -o) may be mistaken 
>> for linker options
>
> Maybe better
>
> # the line "mkdir ... && msgfmt ..." contains no linker options
OK will use.
Show 15 quoted lines
>
>> +            next;
>> +        }
>> +
>> +        if ($text =~ /^msgfmt /) {
>> +            # options to the Portable Object translations in the 
>> line
>> +            # mkdir -p po/... && msgfmt ... (eg -o) may be mistaken 
>> for linker options
>> +            next;
>> +        }
>
> These two if clauses do the same, maybe call it
>
> if ($test =~ /^(mkdir|msgfmt) /)
That's good, will use.
Show 8 quoted lines
>
> Ciao,
> Johannes
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Philip Oakley· Nov 20, 2014, 23:38 UTC · re: Philip Oakley · lore
    the engine.pl script barfs on the properly quoted space
    in filename options prevalent on Windows. Use quotewords()
    rather than split() to separate such options.
    Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
 contrib/buildsystems/engine.pl | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl
index 9144ea7..8e41808 100755
--- a/contrib/buildsystems/engine.pl
+++ b/contrib/buildsystems/engine.pl
@@ -12,6 +12,7 @@ use File::Basename;
 use File::Spec;
 use Cwd;
 use Generators;
+use Text::ParseWords;
 
 my (%build_structure, %compile_options, @makedry);
 my $out_dir = getcwd();
@@ -243,7 +244,8 @@ sub removeDuplicates
 sub handleCompileLine
 {
     my ($line, $lineno) = @_;
-    my @parts = split(' ', $line);
+    # my @parts = split(' ', $line);
+    my @parts = quotewords('\s+', 0, $line);
     my $sourcefile;
     shift(@parts); # ignore cmd
     while (my $part = shift @parts) {
@@ -277,7 +279,8 @@ sub handleLibLine
     my (@objfiles, @lflags, $libout, $part);
     # kill cmd and rm 'prefix'
     $line =~ s/^rm -f .* && .* rcs //;
-    my @parts = split(' ', $line);
+    # my @parts = split(' ', $line);
+    my @parts = quotewords('\s+', 0, $line);
     while ($part = shift @parts) {
         if ($part =~ /^-/) {
             push(@lflags, $part);
@@ -318,7 +321,9 @@ sub handleLinkLine
 {
     my ($line, $lineno) = @_;
     my (@objfiles, @lflags, @libs, $appout, $part);
-    my @parts = split(' ', $line);
+    # my @parts = split(' ', $line);
+    my @parts = quotewords('\s+', 0, $line);
+
     shift(@parts); # ignore cmd
     while ($part = shift @parts) {
         if ($part =~ /^-IGNORE/) {

-- 
1.9.4.msysgit.0

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Johannes Schindelin· Nov 21, 2014, 09:42 UTC · re: Philip Oakley · lore

Re: [RFC 2/4] Properly accept quoted space in filenames

Hi Philip,
apart from adding dead code:
On Thu, 20 Nov 2014, Philip Oakley wrote:
Show 12 quoted lines
> diff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl
> index 9144ea7..8e41808 100755
> --- a/contrib/buildsystems/engine.pl
> +++ b/contrib/buildsystems/engine.pl
> @@ -243,7 +244,8 @@ sub removeDuplicates
>  sub handleCompileLine
>  {
>      my ($line, $lineno) = @_;
> -    my @parts = split(' ', $line);
> +    # my @parts = split(' ', $line);
> +    my @parts = quotewords('\s+', 0, $line);
>      my $sourcefile;
(i.e. the commented-out line) it looks good to me.

Ciao, Johannes

Junio C Hamano· Nov 21, 2014, 22:34 UTC · re: Philip Oakley · lore

Re: [RFC 2/4] Properly accept quoted space in filenames

Philip Oakley <philipoakley@iee.org> writes:
Show 6 quoted lines
>  sub handleCompileLine
>  {
>      my ($line, $lineno) = @_;
> -    my @parts = split(' ', $line);
> +    # my @parts = split(' ', $line);
> +    my @parts = quotewords('\s+', 0, $line);

Can somebody enlighten me why/if quotewords is preferrable over shellwords in the context of this patch?

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Philip Oakley· Nov 21, 2014, 23:01 UTC · re: Junio C Hamano · lore

Re: [RFC 2/4] Properly accept quoted space in filenames

From: "Junio C Hamano" <gitster@pobox.com>
Show 11 quoted lines
> Philip Oakley <philipoakley@iee.org> writes:
>
>>  sub handleCompileLine
>>  {
>>      my ($line, $lineno) = @_;
>> -    my @parts = split(' ', $line);
>> +    # my @parts = split(' ', $line);
>> +    my @parts = quotewords('\s+', 0, $line);
>
> Can somebody enlighten me why/if quotewords is preferrable over
> shellwords in the context of this patch?

"No" - Ignorance is bliss ;-) I think my cargo culting was the result of some googling for "quoting perl variables" or some such, which obviously came up with quotewords - I'm happy to take advice on this one!

quotewords did appear to work though back when I wrote this: 86dcfcf (Properly accept quoted space in filenames, 2012-05-06) -- Philip 

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Junio C Hamano· Nov 21, 2014, 23:21 UTC · re: Philip Oakley · lore

Re: [RFC 2/4] Properly accept quoted space in filenames

"Philip Oakley" <philipoakley@iee.org> writes:
Show 20 quoted lines
> From: "Junio C Hamano" <gitster@pobox.com>
>> Philip Oakley <philipoakley@iee.org> writes:
>>
>>>  sub handleCompileLine
>>>  {
>>>      my ($line, $lineno) = @_;
>>> -    my @parts = split(' ', $line);
>>> +    # my @parts = split(' ', $line);
>>> +    my @parts = quotewords('\s+', 0, $line);
>>
>> Can somebody enlighten me why/if quotewords is preferrable over
>> shellwords in the context of this patch?
>
> "No" - Ignorance is bliss ;-) I think my cargo culting was the result
> of some googling for "quoting perl variables" or some such, which
> obviously came up with quotewords - I'm happy to take advice on this
> one!
>
> quotewords did appear to work though back when I wrote this: 86dcfcf
> (Properly accept quoted space in filenames, 2012-05-06)
A quick websearch shows me:
  http://cpansearch.perl.org/src/CHORNY/Text-ParseWords-3.29/ParseWords.pm

and comparing the implementations of the two, the difference boils down to just one line to me.

    sub shellwords {
        my (@lines) = @_;
        my @allwords;
        foreach my $line (@lines) {
            $line =~ s/^\s+//;
            my @words = parse_line('\s+', 0, $line);
            pop @words if (@words and !defined $words[-1]);
            return() unless (@words || !length($line));
            push(@allwords, @words);
        }
        return(@allwords);
    }

In quotewords, the call to parse_line uses $keep not hardcoded 0 (which would not make any difference in the context of your patch), and it assumes parse_line() never returns a singleton "undef" so the line "pop ... if @words is a list with 'undef' as its sole element" is missing.

Of course, the caller would become smaller and sweeter, i.e.
    my @parts = shellwords($line);

I am not familiar with if Perl folks have certain convention to decide when to use which one, though ;-)

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Philip Oakley· Nov 20, 2014, 23:38 UTC · re: Philip Oakley · lore

[RFC 3/4] engine.pl: split the .o and .obj processing

Commit 4b623d80f7352 added an .obj file (invalidcontinue.obj) which was not processed correctly.

The generate engine then mistakenly did a s/.o/.c/ to create a request to compile invalidcontinue.cbj.

Split the '/\.(o|obj)$' in engine.pl#L353 into:
        } elsif ($part =~ /\.o$/) { # was '/\.(o|obj)$'
            push(@objfiles, $part);
        } elsif ($part =~ /\.obj$/) { # was '/\.(o|obj)$'
            # push(@objfiles, $part); # do nothing
        } else {
And correct the substitution to only operate on .o files.
Also report all errors rather than dieing on the first
 $ git describe 4b623d80f7352
 v1.9.1-1-g4b623d8
The problem exists for V1.9.2 onward
Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
 contrib/buildsystems/engine.pl | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl
index 8e41808..16c3dd5 100755
--- a/contrib/buildsystems/engine.pl
+++ b/contrib/buildsystems/engine.pl
@@ -264,7 +264,9 @@ sub handleCompileLine
         } elsif ($part =~ /\.(c|cc|cpp)$/) {
             $sourcefile = $part;
         } else {
-            die "Unhandled compiler option @ line $lineno: $part";
+            print "full line: $line\n";
+            print "A:-Unhandled compiler option @ line $lineno: $part\n"; # die (if !DEBUG)
+#            die "Unhandled compiler option @ line $lineno: $part";
         }
     }
     @{$compile_options{"${sourcefile}_CFLAGS"}} = @cflags;
@@ -290,14 +292,15 @@ sub handleLibLine
             $libout = $part;
             $libout =~ s/\.a$//;
         } else {
-            die "Unhandled lib option @ line $lineno: $part";
+            print "A:-Unhandled lib option @ line $lineno: $part\n"; # die (if !DEBUG)
+#           die "Unhandled lib option @ line $lineno: $part";
         }
     }
 #    print "LibOut: '$libout'\nLFlags: @lflags\nOfiles: @objfiles\n";
 #    exit(1);
     foreach (@objfiles) {
         my $sourcefile = $_;
-        $sourcefile =~ s/\.o/.c/;
+        $sourcefile =~ s/\.o$/.c/;
         push(@sources, $sourcefile);
         push(@cflags, @{$compile_options{"${sourcefile}_CFLAGS"}});
         push(@defines, @{$compile_options{"${sourcefile}_DEFINES"}});
@@ -343,8 +346,10 @@ sub handleLinkLine
         } elsif ($part =~ /\.(a|lib)$/) {
             $part =~ s/\.a$/.lib/;
             push(@libs, $part);
-        } elsif ($part =~ /\.(o|obj)$/) {
+        } elsif ($part =~ /\.o$/) { # was '/\.(o|obj)$'
             push(@objfiles, $part);
+        } elsif ($part =~ /\.obj$/) { # was '/\.(o|obj)$'
+            # push(@objfiles, $part); # do nothing
         } else {
             die "Unhandled lib option @ line $lineno: $part";
         }
@@ -353,7 +358,7 @@ sub handleLinkLine
 #    exit(1);
     foreach (@objfiles) {
         my $sourcefile = $_;
-        $sourcefile =~ s/\.o/.c/;
+        $sourcefile =~ s/\.o$/.c/;
         push(@sources, $sourcefile);
         push(@cflags, @{$compile_options{"${sourcefile}_CFLAGS"}});
         push(@defines, @{$compile_options{"${sourcefile}_DEFINES"}});

-- 
1.9.4.msysgit.0

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Johannes Schindelin· Nov 21, 2014, 09:48 UTC · re: Philip Oakley · lore

Re: [RFC 3/4] engine.pl: split the .o and .obj processing

Hi Philip,
On Thu, 20 Nov 2014, Philip Oakley wrote:
Show 5 quoted lines
> Commit 4b623d80f7352 added an .obj file (invalidcontinue.obj) which was not
> processed correctly.
> 
> The generate engine then mistakenly did a s/.o/.c/  to create a request
> to compile invalidcontinue.cbj.
This is good. However, this:
Show 7 quoted lines
> Split the '/\.(o|obj)$' in engine.pl#L353 into:
> 
>         } elsif ($part =~ /\.o$/) { # was '/\.(o|obj)$'
>             push(@objfiles, $part);
>         } elsif ($part =~ /\.obj$/) { # was '/\.(o|obj)$'
>             # push(@objfiles, $part); # do nothing
>         } else {

just repeats what the diff says, so it is unnecessary in the commit message.

Show 12 quoted lines
> diff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl
> index 8e41808..16c3dd5 100755
> --- a/contrib/buildsystems/engine.pl
> +++ b/contrib/buildsystems/engine.pl
> @@ -264,7 +264,9 @@ sub handleCompileLine
>          } elsif ($part =~ /\.(c|cc|cpp)$/) {
>              $sourcefile = $part;
>          } else {
> -            die "Unhandled compiler option @ line $lineno: $part";
> +            print "full line: $line\n";
> +            print "A:-Unhandled compiler option @ line $lineno: $part\n"; # die (if !DEBUG)
> +#            die "Unhandled compiler option @ line $lineno: $part";

This needs to be backed out. I agree that it is nice to get going and to debug, so I would split it out into its own commit while working on the branch, but then drop it before submitting.

Show 6 quoted lines
> @@ -290,14 +292,15 @@ sub handleLibLine
>[...]
>      foreach (@objfiles) {
>          my $sourcefile = $_;
> -        $sourcefile =~ s/\.o/.c/;
> +        $sourcefile =~ s/\.o$/.c/;

Ah, I see from the context that my earlier comment about the white-space delimiter was wrong: at this stage, we already have split the list. So this is groovy.

Show 9 quoted lines
> @@ -343,8 +346,10 @@ sub handleLinkLine
>          } elsif ($part =~ /\.(a|lib)$/) {
>              $part =~ s/\.a$/.lib/;
>              push(@libs, $part);
> -        } elsif ($part =~ /\.(o|obj)$/) {
> +        } elsif ($part =~ /\.o$/) { # was '/\.(o|obj)$'
>              push(@objfiles, $part);
> +        } elsif ($part =~ /\.obj$/) { # was '/\.(o|obj)$'
> +            # push(@objfiles, $part); # do nothing
How about the following instead?
+		} elsif ($part eq 'invalidcontinue.obj') {
+			# ignore
 		} elsif ($part =~ /\.(o|obj)$/) {

? After all, this change is really only about handling the newly introduced invalidcontinue.obj correctly.

Ciao, Johannes

Philip Oakley· Nov 21, 2014, 19:57 UTC · re: Johannes Schindelin · lore

Re: [RFC 3/4] engine.pl: split the .o and .obj processing

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Show 24 quoted lines
> Hi Philip,
>
> On Thu, 20 Nov 2014, Philip Oakley wrote:
>
>> Commit 4b623d80f7352 added an .obj file (invalidcontinue.obj) which 
>> was not
>> processed correctly.
>>
>> The generate engine then mistakenly did a s/.o/.c/  to create a 
>> request
>> to compile invalidcontinue.cbj.
>
> This is good. However, this:
>
>> Split the '/\.(o|obj)$' in engine.pl#L353 into:
>>
>>         } elsif ($part =~ /\.o$/) { # was '/\.(o|obj)$'
>>             push(@objfiles, $part);
>>         } elsif ($part =~ /\.obj$/) { # was '/\.(o|obj)$'
>>             # push(@objfiles, $part); # do nothing
>>         } else {
>
> just repeats what the diff says, so it is unnecessary in the commit
> message.
OK.
Show 21 quoted lines
>
>> diff --git a/contrib/buildsystems/engine.pl 
>> b/contrib/buildsystems/engine.pl
>> index 8e41808..16c3dd5 100755
>> --- a/contrib/buildsystems/engine.pl
>> +++ b/contrib/buildsystems/engine.pl
>> @@ -264,7 +264,9 @@ sub handleCompileLine
>>          } elsif ($part =~ /\.(c|cc|cpp)$/) {
>>              $sourcefile = $part;
>>          } else {
>> -            die "Unhandled compiler option @ line $lineno: $part";
>> +            print "full line: $line\n";
>> +            print "A:-Unhandled compiler option @ line $lineno: 
>> $part\n"; # die (if !DEBUG)
>> +#            die "Unhandled compiler option @ line $lineno: $part";
>
> This needs to be backed out. I agree that it is nice to get going and 
> to
> debug, so I would split it out into its own commit while working on 
> the
> branch, but then drop it before submitting.

I'll probably split this "improvement in error reporting" (by providing the full offending line) into a separate patch, on the basis that we want (Windows) folks to start helping, so let's make the error messages more useful to those who don't know these scripting languages yet.

Show 28 quoted lines
>
>> @@ -290,14 +292,15 @@ sub handleLibLine
>>[...]
>>      foreach (@objfiles) {
>>          my $sourcefile = $_;
>> -        $sourcefile =~ s/\.o/.c/;
>> +        $sourcefile =~ s/\.o$/.c/;
>
> Ah, I see from the context that my earlier comment about the 
> white-space
> delimiter was wrong: at this stage, we already have split the list. So
> this is groovy.
>
>> @@ -343,8 +346,10 @@ sub handleLinkLine
>>          } elsif ($part =~ /\.(a|lib)$/) {
>>              $part =~ s/\.a$/.lib/;
>>              push(@libs, $part);
>> -        } elsif ($part =~ /\.(o|obj)$/) {
>> +        } elsif ($part =~ /\.o$/) { # was '/\.(o|obj)$'
>>              push(@objfiles, $part);
>> +        } elsif ($part =~ /\.obj$/) { # was '/\.(o|obj)$'
>> +            # push(@objfiles, $part); # do nothing
>
> How about the following instead?
>
> + } elsif ($part eq 'invalidcontinue.obj') {
> + # ignore
>  } elsif ($part =~ /\.(o|obj)$/) {

Looks good, I'll use that (after deciding whether .obj files should be expected in a 'make' output anyway)

>
> ? After all, this change is really only about handling the newly
> introduced invalidcontinue.obj correctly.
Did it ever handle .obj's correctly (rhet) - I dunno.
Show 8 quoted lines
>
> Ciao,
> Johannes
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Johannes Schindelin· Nov 23, 2014, 15:28 UTC · re: Philip Oakley · lore

Re: [RFC 3/4] engine.pl: split the .o and .obj processing

Hi,
On Fri, 21 Nov 2014, Philip Oakley wrote:
Show 22 quoted lines
> From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
>
> > On Thu, 20 Nov 2014, Philip Oakley wrote:
> >
> > > @@ -343,8 +346,10 @@ sub handleLinkLine
> > >          } elsif ($part =~ /\.(a|lib)$/) {
> > >              $part =~ s/\.a$/.lib/;
> > >              push(@libs, $part);
> > > -        } elsif ($part =~ /\.(o|obj)$/) {
> > > +        } elsif ($part =~ /\.o$/) { # was '/\.(o|obj)$'
> > >              push(@objfiles, $part);
> > > +        } elsif ($part =~ /\.obj$/) { # was '/\.(o|obj)$'
> > > +            # push(@objfiles, $part); # do nothing
> >
> > How about the following instead?
> >
> > + } elsif ($part eq 'invalidcontinue.obj') {
> > + # ignore
> >  } elsif ($part =~ /\.(o|obj)$/) {
> 
> Looks good, I'll use that (after deciding whether .obj files should be
> expected in a 'make' output anyway)

My idea to hardcode invalidcontinue.obj was that I'd rather see a failure if an unexpected .obj is seen there. But I realize that my suggested change does not exactly accomplish that.

Ciao, Johannes

Philip Oakley· Nov 23, 2014, 21:58 UTC · re: Johannes Schindelin · lore

Re: Re: [RFC 3/4] engine.pl: split the .o and .obj processing

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Show 15 quoted lines
>> > How about the following instead?
>> >
>> > + } elsif ($part eq 'invalidcontinue.obj') {
>> > + # ignore
>> >  } elsif ($part =~ /\.(o|obj)$/) {
>>
>> Looks good, I'll use that (after deciding whether .obj files should
>> be
>> expected in a 'make' output anyway)
>
> My idea to hardcode invalidcontinue.obj was that I'd rather see a
> failure
> if an unexpected .obj is seen there. But I realize that my suggested
> change does not exactly accomplish that.
>

I've decided to include the hard code suggested, and after a few tests decided it was probably worth keeping the .obj processing, even though we don't expect any.

At least I've now managed to generate a .sln project, though it won't compile because of a number of other changes, and I haven't got to the bottom of them all.

Notes: Revert "Windows: correct detection of EISDIR in mingw_open()" git\compat\mingw.c(315) : error C2065: 'O_ACCMODE' : undeclared identifier (neither VS2008 nor VS2010 declare 'O_ACCMODE'). add an #ifdef _MSC_VER to define this if we are in MSVC.

Revert "mingw.h: add dummy functions for sigset_t operations" commit 4e6d207c45. the Additional Note: to http://www.spinics.net/lists/git/msg240248.html indicates that this breaks MSVC. I think it will also need an #ifdef to see if we are in MSVC.

I've also found a problem with my setup (msysgit 1.7.9 for make) barfing on the commit/ee9be06 "perl: detect new files in MakeMaker builds" which 'make -n' says 'No rule to make target `PM.stamp', needed by `perl.mak'.

There was also LINK : warning LNK4098: defaultlib 'MSVCRT' conflicts with use of other libs; use /NODEFAULTLIB:library.

I've also got notes from way back that there was build order problem with 'vcs-svn_lib.lib' - probably a sort order issue. IIRC at that time, I had to compile the project twice so that the lib was visible the second time.

So there's plenty to go at ;-) I'm going to be away this coming week, So at least I have the whole next cycle to make some progress.

--
Philip
-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Philip Oakley· Nov 20, 2014, 23:38 UTC · re: Philip Oakley · lore

[RFC 4/4] Improve layout and reference msvc-build script

Layout the 'either/or' with more white space to clarify which alternatives are matched up.

Reference the build script which automates one sequence of options.
Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
 compat/vcbuild/README | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/compat/vcbuild/README b/compat/vcbuild/README
index df8a657..f4f6723 100644
--- a/compat/vcbuild/README
+++ b/compat/vcbuild/README
@@ -3,20 +3,24 @@ The Steps of Build Git with VS2008
 1. You need the build environment, which contains the Git dependencies
    to be able to compile, link and run Git with MSVC.
 
-   You can either use the binary repository:
+   You can either:
+      use the binary repository:
 
        WWW: http://repo.or.cz/w/msvcgit.git
        Git: git clone git://repo.or.cz/msvcgit.git
        Zip: http://repo.or.cz/w/msvcgit.git?a=snapshot;h=master;sf=zip
 
-   and call the setup_32bit_env.cmd batch script before compiling Git,
-   (see repo/package README for details), or the source repository:
+      and call the setup_32bit_env.cmd batch script before compiling Git,
+     (see repo/package README for details),
+
+   or:
+      use the source repository:
 
        WWW: http://repo.or.cz/w/gitbuild.git
        Git: git clone git://repo.or.cz/gitbuild.git
        Zip: (None, as it's a project with submodules)
 
-   and build the support libs as instructed in that repo/package.
+     and build the support libs as instructed in that repo/package.
 
 2. Ensure you have the msysgit environment in your path, so you have
    GNU Make, bash and perl available.
@@ -33,18 +37,27 @@ The Steps of Build Git with VS2008
        make common-cmds.h
    to generate the common-cmds.h file needed to compile git.
 
-4. Then either build Git with the GNU Make Makefile in the Git projects
-   root
+4. Then either
+
+     build Git with the GNU Make Makefile in the Git projects root
        make MSVC=1
-   or generate Visual Studio solution/projects (.sln/.vcproj) with the
+   or
+
+   generate Visual Studio solution/projects (.sln/.vcproj) with the
    command
        perl contrib/buildsystems/generate -g Vcproj
    and open and build the solution with the IDE
        devenv git.sln /useenv
-   or build with the IDE build engine directly from the command line
+     or
+
+   build with the IDE build engine directly from the command line
        devenv git.sln /useenv /build "Release|Win32"
    The /useenv option is required, so Visual Studio picks up the
    environment variables for the support libraries required to build
    Git, which you set up in step 1.
 
 Done!
+
+Or, use the msvc-build script; also in /msysgit/bin/msvc-build
+
+For those with VS2010: You may need to associate the generated .sln file with VS2010 rather than the MS version selector program.

-- 
1.9.4.msysgit.0

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Johannes Schindelin· Nov 21, 2014, 09:51 UTC · re: Philip Oakley · lore

Re: [RFC 4/4] Improve layout and reference msvc-build script

Hi Philip,
On Thu, 20 Nov 2014, Philip Oakley wrote:
> [...]
> +Or, use the msvc-build script; also in /msysgit/bin/msvc-build

As I mentioned before, from a Git Bash on Windows, the path is /bin/msvc-build (no /msysgit/). That is quite likely to stay the same with the upcoming Git for Windows SDK, too (once I add the file to the SDK).

By the way, I think we should also start thinking about a Jenkins job to verify that upstream changes such as invalidcontinue.obj do not break the MSVC build. Please let me know if you want to give it a try, I have a Windows Azure instance with a Jenkins instance, sponsored by Microsoft.

Ciao, Johannes

Philip Oakley· Nov 21, 2014, 19:57 UTC · re: Johannes Schindelin · lore

Re: [RFC 4/4] Improve layout and reference msvc-build script

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Show 12 quoted lines
> Hi Philip,
>
> On Thu, 20 Nov 2014, Philip Oakley wrote:
>
>> [...]
>> +Or, use the msvc-build script; also in /msysgit/bin/msvc-build
>
> As I mentioned before, from a Git Bash on Windows, the path is
> /bin/msvc-build (no /msysgit/). That is quite likely to stay the same 
> with
> the upcoming Git for Windows SDK, too (once I add the file to the 
> SDK).

I was trying to identify the source file (as I see it in my Msysgit install). However it will all need tidying to distinguish what is seen if you have G4W-SDK, rather just installing the new G4W (when ready)

Show 8 quoted lines
>
> By the way, I think we should also start thinking about a Jenkins job 
> to
> verify that upstream changes such as invalidcontinue.obj do not break 
> the
> MSVC build. Please let me know if you want to give it a try, I have a
> Windows Azure instance with a Jenkins instance, sponsored by 
> Microsoft.

This is not something I'm familiar with, but it's something I could have a look at. On my list is also the 'git bundle' symref problem, which we've got a solution for that 'just' needs coding, along with thoughts about narrow/sparse checkout/clone/fetch.

One of the confusions I have is the distinction between building with the MSVC compiler (cl command line) and building within VS2008, and whether they are different things (i.e. does VS2008 bring extra baggage?). -- Philip

>
> Ciao,
> Johannes
> 
-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Johannes Schindelin· Nov 21, 2014, 09:38 UTC · re: Philip Oakley · lore

Re: [RFC 0/4] Fix the Visual Studio 2008 .sln generator

Hi Philip,
On Thu, 20 Nov 2014, Philip Oakley wrote:
> Are the patches going in the right direction?
Yes.
A couple of general comments:
- please do not comment out code. Just remove it.
- the first three commit messages look funny, being indented by 4
  spaces... unintentional?
> Is the processing of the .obj file in engine.pl sensible?

Yes, but instead of adding dead code, it would be better to use the comment "# ignore". I would also strongly suggest to hard-code the complete file name instead of just the extension. We know exactly which file name we want to ignore, and if there should be another .obj file eventually, it would be wrong to ignore it, too.

> and the extra care with s/\.o$/.c/ avoiding s/obj/cbj/.

Technically, you need to use a group \($\|[ \t]\), but I think that that would be overkill.

> Does it affect the Qmake capability? (I've no idea)

Frankly, neither do I ;-) But since you touched only engine.pl, I would expect Qmake not to be affected at all, right?

> Is the quoting of filenames correct? (my perl foo is cargo cult!)
IANAPME (I am not a Perl Monk either), but it looks good to me.
> I've also updated the vcbuild/README to mention Msysgit (which
> will be replaced soon by the newer/better Git-for-windows/SDK
> (https://github.com/git-for-windows/sdk), but the benefits still
> apply.

The path you used is /msysgit/bin/msvc-build, but the real path would be /bin/msvc-build.

> Obviously, the patches will need squashing together,
To the contrary, I like the current separation of concerns.

Ciao, Johannes

Philip Oakley· Nov 21, 2014, 19:57 UTC · re: Johannes Schindelin · lore

Re: [RFC 0/4] Fix the Visual Studio 2008 .sln generator

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Show 7 quoted lines
> Hi Philip,
>
> On Thu, 20 Nov 2014, Philip Oakley wrote:
>
>> Are the patches going in the right direction?
>
> Yes.
Magic. Glad of the confirmation
>
> A couple of general comments:
>
> - please do not comment out code. Just remove it.
It's my debugging style until I get it working ;-0
>
> - the first three commit messages look funny, being indented by 4
>  spaces... unintentional?

I think that was from one of the tools I was using at the time. They'll be tidied and reflowed for the final version(s).

Show 6 quoted lines
>
>> Is the processing of the .obj file in engine.pl sensible?
>
> Yes, but instead of adding dead code, it would be better to use the
> comment "# ignore". I would also strongly suggest to hard-code the
> complete file name instead of just the extension.
True, I realised that after posting (isnt it always the way).
> We know exactly which
> file name we want to ignore, and if there should be another .obj file
> eventually, it would be wrong to ignore it, too.

My concern was more about why a 'make' would produce .obj files (rather than .o files) in the first place. As I understand it we should never see .obj files generated from make with the exception of these ones that are library files implicitly known to MSVC - any thoughts?

Show 12 quoted lines
>
>> and the extra care with s/\.o$/.c/ avoiding s/obj/cbj/.
>
> Technically, you need to use a group \($\|[ \t]\), but I think that 
> that
> would be overkill.
>
>> Does it affect the Qmake capability? (I've no idea)
>
> Frankly, neither do I ;-) But since you touched only engine.pl, I 
> would
> expect Qmake not to be affected at all, right?

As I understood the call sequence merry go round, the engine can pull in either function depending on command line options, hence the open question. But like you say, why expect it to be affected, and no-one has mentioned on list for years;-)

>
>> Is the quoting of filenames correct? (my perl foo is cargo cult!)
>
> IANAPME (I am not a Perl Monk either), but it looks good to me.
OK.
Show 13 quoted lines
>
>> I've also updated the vcbuild/README to mention Msysgit (which
>> will be replaced soon by the newer/better Git-for-windows/SDK
>> (https://github.com/git-for-windows/sdk), but the benefits still
>> apply.
>
> The path you used is /msysgit/bin/msvc-build, but the real path would 
> be
> /bin/msvc-build.
>
>> Obviously, the patches will need squashing together,
>
> To the contrary, I like the current separation of concerns.

At the moment the first patch doesn't fully cure the .sln build, but there's no test anyway so bisecting would be problematic.

I'm happy to keep them small separate and clean ;-)
Show 8 quoted lines
>
> Ciao,
> Johannes
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

← back to recent threads