Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]

[Stefan Beller]

On Thu, Nov 2, 2017 at 1:01 AM, Jeff King  wrote:
> On Thu, Nov 02, 2017 at 04:48:45PM +0900, Junio C Hamano wrote:
>
>> Jeff King  writes:
>>
>> > If the clutter is too much, it could also go into a git-notes ref
>> > (that's not already implemented, but it seems like it would be pretty
>> > easy to teach "git am" to do that).
>>
>> FWIW, that is what I use a notes ref "amlog" for.
>
> Right, I completely forgot that you were already doing that. So jumping
> to the thread for a commit is basically:
>
>   $browser $(git notes --ref=amlog show $commit |
>  perl -pe 's{Message-Id: 
> <(.*)>}{https://public-inbox.org/git/$1/t}')
>
> (Or whichever archive you prefer to use). Thanks for the reminder.
>

Thanks from my side as well. I did not pay attention when this was
introduced and see the notes ref for the first time today.

I'll incorporate that into my digging repertoire.

Thanks,
Stefan

Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]

[Jeff King]

On Thu, Nov 02, 2017 at 04:48:45PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > If the clutter is too much, it could also go into a git-notes ref
> > (that's not already implemented, but it seems like it would be pretty
> > easy to teach "git am" to do that).
> 
> FWIW, that is what I use a notes ref "amlog" for.

Right, I completely forgot that you were already doing that. So jumping
to the thread for a commit is basically:

  $browser $(git notes --ref=amlog show $commit |
 perl -pe 's{Message-Id: 
<(.*)>}{https://public-inbox.org/git/$1/t}')

(Or whichever archive you prefer to use). Thanks for the reminder.

-Peff

Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]

[Junio C Hamano]

Jeff King  writes:

> If the clutter is too much, it could also go into a git-notes ref
> (that's not already implemented, but it seems like it would be pretty
> easy to teach "git am" to do that).

FWIW, that is what I use a notes ref "amlog" for.

Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]

[Jeff King]

On Wed, Nov 01, 2017 at 03:31:42PM -0700, Stefan Beller wrote:

> On Wed, Nov 1, 2017 at 9:42 AM, Stefan Beller  wrote:
> 
> >> So it may make more sense just to cross-reference those merges with the
> >> topics that spawned them on the mailing list. I.e., instead of copying
> >> the cover letter contents, just record the message-id (and update it
> >> whenever a new iteration of a topic is picked up via "git am"). That
> >> lets you get the cover letter information _and_ see any discussion
> >> or review around the patch.
> >
> > That sounds good.
> 
> Actually I just found out about `am.messageId`, which adds the individual
> message id as a footer. Maybe that is good enough? (Though it would
> clutter every commit, not just the merge commits)

It also means digging around to find the apex of the thread (though
generally that can be done automatically with sufficiently smart
tooling; I think public-inbox can do it pretty easily).

I also like the idea that I could read "log --first-parent" to get an
overview of the topics (with links). But probably associating a message
id with each patch smooths out a lot of corner cases (it sidesteps the
"where do you store it until the commit is made" question, and it
works when there's no cover letter). And it gives enough hint for other
software to figure out everything else.

If the clutter is too much, it could also go into a git-notes ref
(that's not already implemented, but it seems like it would be pretty
easy to teach "git am" to do that).

For a while, Thomas Rast used a script to heuristically create that
mapping via git-notes after the fact. But if "git am" just did it
automatically on behalf of Junio, that would be more robust.

I will admit that I found I didn't use the mapping generated by Thomas's
script all that much. But I do keep a local mailing list index and often
search for the commit's subject as a mail subject, which roughly
accomplishes the same thing.

-Peff

Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]

[Stefan Beller]

On Wed, Nov 1, 2017 at 9:42 AM, Stefan Beller  wrote:

>> So it may make more sense just to cross-reference those merges with the
>> topics that spawned them on the mailing list. I.e., instead of copying
>> the cover letter contents, just record the message-id (and update it
>> whenever a new iteration of a topic is picked up via "git am"). That
>> lets you get the cover letter information _and_ see any discussion
>> or review around the patch.
>
> That sounds good.
>

Actually I just found out about `am.messageId`, which adds the individual
message id as a footer. Maybe that is good enough? (Though it would
clutter every commit, not just the merge commits)

Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]

[Stefan Beller]

On Wed, Nov 1, 2017 at 12:14 AM, Jeff King  wrote:
> On Mon, Oct 30, 2017 at 11:29:37AM -0700, Stefan Beller wrote:
>
>> > I can live with fancily-formatted cover letters. BUT. I would say if
>> > your cover letter is getting quite long, you might consider whether some
>> > of its content ought to be going elsewhere (either into commit messages
>> > themselves, or into a design document or other place inside the repo).
>>
>> I am of the opinion that in an ideal workflow the cover letter would be
>> part of the merge commit message. It may even be editorialized or
>> annotated by the maintainer performing the merge, but not necessarily so.
>>
>> Currently I rarely pay attention to merges, because they are not exciting
>> (in a good way). I mostly know the texts that Junio puts into the merge
>> message[1] from the cooking email, and otherwise there is not much
>> information added there.
>
> Yes, I'd agree that for some topics it would be nice for the merge
> message to give any "big picture" details that wouldn't have made sense
> inside a single commit message.
>
>> However looking at *what* Junio puts there, is "why is this worthwhile
>> to merge from the *projects* point of view?", whereas the cover letter
>> most of the time talks about "Why the *contributor* wants this merged".
>> Often these are subtly different, so it would be nice to have these
>> two competing views around.
>
> Yes, there's really no reason we couldn't have both (e.g., Junio's
> maintainer synopsis followed by a marker, and then the original author's
> cover letter).
>
> The workflow within git is a little awkward, though. You'd want to pick
> up the cover letter information via "git am" when the branch is applied.
> But it doesn't go into a commit message until the merge. So where is it
> stored in the meantime? You'd need a branch->msg key/value store of some
> kind.

branch.description already exists on the contributors side.
Maybe we can teach git-am to populate that field with either the
message-id only (of the coverletter), or the cover letter text.

Or we introduce a git-am hook, that could populate the value
of that setting.

Once we have that setting there, our man page of git-merge claims
merging pays attention to that setting via git-fmt-merge-msg.

I guess if we put these pieces in place, it may be easier to convince
Junio to try out that workflow.

> Junio's workflow now actually uses the "pu" merges as the storage
> location while a topic is working its way up. But there's a period
> between "am" and running the integration cycle where it would need to go
> somewhere else.

An empty commit on top is a clunky idea. (Though from the data model,
that empty commit "just learns about a new parent" in the merge process).
I think the branch description field will do.

> One other consideration is that the cover letter may get updated between
> rounds (e.g., because you changed patches in response to review, or even
> to discuss alternatives that came up and were rejected). That places a
> burden on the maintainer to update the prospective merge-message.

if there was a git-am hook, that could be smart enough to *always*
update the branch description to the latest cover letter, (or even just
the latest patch of the series, if just the last patch changed)

> So it may make more sense just to cross-reference those merges with the
> topics that spawned them on the mailing list. I.e., instead of copying
> the cover letter contents, just record the message-id (and update it
> whenever a new iteration of a topic is picked up via "git am"). That
> lets you get the cover letter information _and_ see any discussion
> or review around the patch.

That sounds good.

Stefan

Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]

[Jeff King]

On Mon, Oct 30, 2017 at 11:29:37AM -0700, Stefan Beller wrote:

> > I can live with fancily-formatted cover letters. BUT. I would say if
> > your cover letter is getting quite long, you might consider whether some
> > of its content ought to be going elsewhere (either into commit messages
> > themselves, or into a design document or other place inside the repo).
> 
> I am of the opinion that in an ideal workflow the cover letter would be
> part of the merge commit message. It may even be editorialized or
> annotated by the maintainer performing the merge, but not necessarily so.
> 
> Currently I rarely pay attention to merges, because they are not exciting
> (in a good way). I mostly know the texts that Junio puts into the merge
> message[1] from the cooking email, and otherwise there is not much
> information added there.

Yes, I'd agree that for some topics it would be nice for the merge
message to give any "big picture" details that wouldn't have made sense
inside a single commit message.

> However looking at *what* Junio puts there, is "why is this worthwhile
> to merge from the *projects* point of view?", whereas the cover letter
> most of the time talks about "Why the *contributor* wants this merged".
> Often these are subtly different, so it would be nice to have these
> two competing views around.

Yes, there's really no reason we couldn't have both (e.g., Junio's
maintainer synopsis followed by a marker, and then the original author's
cover letter).

The workflow within git is a little awkward, though. You'd want to pick
up the cover letter information via "git am" when the branch is applied.
But it doesn't go into a commit message until the merge. So where is it
stored in the meantime? You'd need a branch->msg key/value store of some
kind.

Junio's workflow now actually uses the "pu" merges as the storage
location while a topic is working its way up. But there's a period
between "am" and running the integration cycle where it would need to go
somewhere else.

One other consideration is that the cover letter may get updated between
rounds (e.g., because you changed patches in response to review, or even
to discuss alternatives that came up and were rejected). That places a
burden on the maintainer to update the prospective merge-message.

So it may make more sense just to cross-reference those merges with the
topics that spawned them on the mailing list. I.e., instead of copying
the cover letter contents, just record the message-id (and update it
whenever a new iteration of a topic is picked up via "git am"). That
lets you get the cover letter information _and_ see any discussion
or review around the patch. (But it has the same "where does this
message-id go until the merge-commit is created" question).

> > I actually have the opposite opinion. I find it annoying to have to wade
> > through the same unchanged content for each round just to find the
> > little snippet of "here's what's changed".
> 
> Would you be happier if the "What changed?" goes first[2]?

Yes, I think that would help. And marking the start of "old" information
clearly so that the reader knows when they can stop looking.

But then links do that pretty well, too (it's easy to choose whether to
follow them or not).

-Peff

On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]

[Stefan Beller]

On Mon, Oct 30, 2017 at 11:08 AM, Jeff King  wrote:
> On Mon, Oct 23, 2017 at 01:26:41PM +0100, Philip Oakley wrote:
>
>> > Totally offtopic, but is it only me who finds these "section
>> > headers" in cover letters from some people irritating and/or
>> > jarring?
>>
>> Personally I find that, for significant patch series, that clearly breaking
>> out these distinct sections is of advantage. At this stage (the very first
>> patch 0/n) there is no specific conversation, so the subject line is a short
>> 'hello' to the topic, and then the contributor is (it is to be hoped)
>> setting out their proposal in a clear manner.
>>
>> So I do like these headings for larger series, though there is some
>> judgement to be made as to when the subject line alone is sufficient.
>
> I can live with fancily-formatted cover letters. BUT. I would say if
> your cover letter is getting quite long, you might consider whether some
> of its content ought to be going elsewhere (either into commit messages
> themselves, or into a design document or other place inside the repo).

I am of the opinion that in an ideal workflow the cover letter would be
part of the merge commit message. It may even be editorialized or
annotated by the maintainer performing the merge, but not necessarily so.

Currently I rarely pay attention to merges, because they are not exciting
(in a good way). I mostly know the texts that Junio puts into the merge
message[1] from the cooking email, and otherwise there is not much
information added there.

However looking at *what* Junio puts there, is "why is this worthwhile
to merge from the *projects* point of view?", whereas the cover letter
most of the time talks about "Why the *contributor* wants this merged".
Often these are subtly different, so it would be nice to have these
two competing views around.

[1] e.g. cf. da15b78e52 Merge branch 'jk/ui-color-always-to-auto'

>> As a separate follow on, one thing that does annoy me is that in subsequent
>> versions of the various patch series, folk tend to drop all explanation of
>> why the series is of any relevance, leaving just the 'changed since last
>> time' part. This means that new readers who try and pick up / review /
>> contribute to a series later on in its development are not told the purpose.
>> When the list is active it can, accidentally, do a disservice to the
>> potential contributors who may feel that only core contributors are able to
>> contribute.
>
> I actually have the opposite opinion. I find it annoying to have to wade
> through the same unchanged content for each round just to find the
> little snippet of "here's what's changed".

Would you be happier if the "What changed?" goes first[2]?
Though I think whether to just reply to the previous version, put an
explicit link or copy the cover letter verbatim from last time is up
for discussion. I tent to think a link ought to be enough, because
those familiar with the topic would not follow it (so they have no
additional burden compared to direct copy), and new comers to
that topic may be ok with links .

[2] I tried following that style, e.g.
https://public-inbox.org/git/20170630205310.7380-1-sbel...@google.com/


> I don't mind following a link to the previous iteration to read the
> back-story if I wasn't involved (it's a good idea to do that anyway to
> see what previous reviews have already discussed).

Such a back story would make an excellent merge message, too,
as it explains the big picture more accurately, often shows
alternatives considered etc.

Stefan

Re: [PATCH 0/6] Create Git/Packet.pm

[Jeff King]

On Mon, Oct 23, 2017 at 01:26:41PM +0100, Philip Oakley wrote:

> > Totally offtopic, but is it only me who finds these "section
> > headers" in cover letters from some people irritating and/or
> > jarring?
> 
> Personally I find that, for significant patch series, that clearly breaking
> out these distinct sections is of advantage. At this stage (the very first
> patch 0/n) there is no specific conversation, so the subject line is a short
> 'hello' to the topic, and then the contributor is (it is to be hoped)
> setting out their proposal in a clear manner.
> 
> So I do like these headings for larger series, though there is some
> judgement to be made as to when the subject line alone is sufficient.

I can live with fancily-formatted cover letters. BUT. I would say if
your cover letter is getting quite long, you might consider whether some
of its content ought to be going elsewhere (either into commit messages
themselves, or into a design document or other place inside the repo).

> As a separate follow on, one thing that does annoy me is that in subsequent
> versions of the various patch series, folk tend to drop all explanation of
> why the series is of any relevance, leaving just the 'changed since last
> time' part. This means that new readers who try and pick up / review /
> contribute to a series later on in its development are not told the purpose.
> When the list is active it can, accidentally, do a disservice to the
> potential contributors who may feel that only core contributors are able to
> contribute.

I actually have the opposite opinion. I find it annoying to have to wade
through the same unchanged content for each round just to find the
little snippet of "here's what's changed".

I don't mind following a link to the previous iteration to read the
back-story if I wasn't involved (it's a good idea to do that anyway to
see what previous reviews have already discussed).

I do often just post my "v2" as a follow-up and assume people can find
the original by following the thread backwards. But I imagine that not
everybody can do so. It's probably a good practice to at least put a
link to the prior version (and also to v1 for the original motivation)
if you're not going to repeat the cover letter in full.

-Peff

Re: [PATCH 0/6] Create Git/Packet.pm

[Johannes Schindelin]

Hi Christian & Junio,

On Mon, 30 Oct 2017, Christian Couder wrote:

> On Mon, Oct 30, 2017 at 1:38 AM, Junio C Hamano  wrote:
> > I've queued this from Dscho; please take it into consideration when
> > you reroll.
> 
> Yeah, I was planning to add something like that, though in Dscho's
> first email the patch was adding:
> 
> +modules += Git/Packet
> 
> and now it's adding:
> 
> > +modules += Git/SVN/Packet

Bah. I should have paid more attention. The original Git/Packet is
correct, of course. My fixup! commit is bogus.

Sorry for the confusion,
Dscho

Re: [PATCH 0/6] Create Git/Packet.pm

[Christian Couder]

Hi,

On Mon, Oct 30, 2017 at 1:38 AM, Junio C Hamano  wrote:
> I've queued this from Dscho; please take it into consideration when
> you reroll.

Yeah, I was planning to add something like that, though in Dscho's
first email the patch was adding:

+modules += Git/Packet

and now it's adding:

> +modules += Git/SVN/Packet

I wonder where the "SVN" directory comes from as in
https://github.com/dscho/git/commits/cc/git-packet-pm my commit
(https://github.com/dscho/git/commit/46032e5f55f6e87d22f9a3c952b5d48a5f100a86)
still creates perl/Git/Packet.pm and not perl/Git/SVN/Packet.pm

Thanks both anyway,
Christian.

Re: [PATCH 0/6] Create Git/Packet.pm

[Junio C Hamano]

I've queued this from Dscho; please take it into consideration when
you reroll.

Thanks.

-- >8 --
From: Johannes Schindelin 
Date: Sun, 29 Oct 2017 16:17:06 +0100
Subject: [PATCH] fixup! Git/Packet.pm: extract parts of t0021/rot13-filter.pl
 for reuse

The patch introducing Git/Packet.pm forgot the NO_PERL_MAKEMAKER part.
Breaking the test suite on Windows.

Signed-off-by: Johannes Schindelin 
---
 perl/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/perl/Makefile b/perl/Makefile
index 15d96fcc7a..4a74a493e6 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -37,6 +37,7 @@ modules += Git/SVN/Editor
 modules += Git/SVN/GlobSpec
 modules += Git/SVN/Log
 modules += Git/SVN/Migration
+modules += Git/SVN/Packet
 modules += Git/SVN/Prompt
 modules += Git/SVN/Ra
 modules += Git/SVN/Utils
-- 
2.15.0-rc2-267-g7d3ed0014a

Re: [PATCH 0/6] Create Git/Packet.pm

[Johannes Schindelin]

Hi Bryan,

On Thu, 26 Oct 2017, Bryan Turner wrote:

> On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller  wrote:
> > On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano  wrote:
> >> Johannes Schindelin  writes:
> >>
> >>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
> >>> the line
> >>>
> >>>   use lib (split(/:/, $ENV{GITPERLLIB}));
> >>>
> >>> splits off the drive letter from the rest of the path. Obviously, this
> >>> fails to Do The Right Thing, and simply points to Yet Another Portability
> >>> Problem with Git's reliance on Unix scripting.
> >>
> >> In our C code, we have "#define PATH_SEP ';'", and encourage our
> >> code to be careful and use it.  Is there something similar for Perl
> >> scripts, I wonder.
> >>
> >> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> >> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> >> the non-platform convention to accomodate the use of split(/:/)
> >> certainly is a workaround, but it does feel dirty.
> >>
> >> It is hard to imagine that we were the first people who wants to
> >> split the value of a variable into a list, where the value is a list
> >> of paths, concatenated into a single string with a delimiter that
> >> may be platform specific.  I wonder if we are going against a best
> >> practice established in the Perl world, simply because we don't know
> >> about it (i.e. basically, it would say "don't split at a colon
> >> because not all world is Unix; use $this_module instead", similar to
> >> "don't split at a slash, use File::Spec instead to extract path
> >> components").
> >>
> >
> > I thought there was a way to do this in File::Spec, but that's only
> > for splitting regular paths, and not for splitting a list of paths
> > separated by ":" or ";"
> >
> > We probably should find a better solution to allow this to work with
> > windows style paths...? I know that python provides os.pathsep, but I
> > haven't seen an equivalent for perl yet.
> >
> > The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
> > maybe we should be using this?
> 
> I was testing this recently on the Perl included with Git for Windows
> and it returns : for the path separator even though it's on Windows,
> so I don't think that would work. The Perl in Git for Windows seems to
> want UNIX-style inputs (something Dscho seemed to allude to in his
> response earlier.). I'm not sure why it's that way, but he probably
> knows.

MSYS2 Perl is essentially Cygwin's Perl ported over to MSYS2. And Cygwin
tries to keep everything as pseudo Unix as possible, to make it easier to
port software (if you think Git's source code is the only source code
woefully unprepared for semicolons as path separators, you just need to
buy me a few beers to hear plenty of war stories).

Ciao,
Dscho

Re: [PATCH 0/6] Create Git/Packet.pm

[Johannes Schindelin]

Hi Junio,

On Thu, 26 Oct 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Note that the correct blib path starts with `C:\BuildAgent\_work` and
> > the line
> >
> > use lib (split(/:/, $ENV{GITPERLLIB}));
> >
> > splits off the drive letter from the rest of the path. Obviously, this
> > fails to Do The Right Thing, and simply points to Yet Another Portability
> > Problem with Git's reliance on Unix scripting.
> 
> In our C code, we have "#define PATH_SEP ';'", and encourage our
> code to be careful and use it.  Is there something similar for Perl
> scripts, I wonder.
> 
> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> the non-platform convention to accomodate the use of split(/:/)
> certainly is a workaround, but it does feel dirty.

It is not only dirty, it *still* does not work with that workaround: Note
that GITPERLLIB is *also* set in the bin-wrappers/*...

And even then, it does not work: somewhere along the way, the path is
converted to a Windows path with backslashes, and for some reason MSYS2
Perl fails to handle that.

The best workaround I found in Git's source code (yes, it took me 2h to
investigate this littly problem, but at least I got an in-depth
understanding of the issue) was to pass BLIB_DIR instead, and not perform
a split but generate the two paths to include in Perl instead. Of course,
that would break out-of-tree usage of GITPERLLIB...

That's why I settled on the out-of-tree workaround that Windows
contributors will have to somehow figure out (as if it was not hard enough
already to contribute to Git for Windows...).

> It is hard to imagine that we were the first people who wants to
> split the value of a variable into a list, where the value is a list
> of paths, concatenated into a single string with a delimiter that
> may be platform specific.

No, the test suite has plenty of use cases for that. It usually works.

The problem is that t0021 contains very complex code that goes back and
forth between the C layer and the scripted layer. At one stage, the
pseudo-Unix paths are converted to Windows paths, with drive prefix and
backslashes, separated by semicolons. And somewhere along the lines, this
cannot be converted back.

I *think* that it happens when the bin-wrapper for git.exe is executed
from inside Git itself, or some such.

> I wonder if we are going against a best practice established in the Perl
> world, simply because we don't know about it (i.e. basically, it would
> say "don't split at a colon because not all world is Unix; use
> $this_module instead", similar to "don't split at a slash, use
> File::Spec instead to extract path components").

We go against best practices by having crucial parts of Git implemented as
Perl scripts. But you knew that ;-)

Ciao,
Dscho

Re: [PATCH 0/6] Create Git/Packet.pm

[Bryan Turner]

On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller  wrote:
> On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano  wrote:
>> Johannes Schindelin  writes:
>>
>>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
>>> the line
>>>
>>>   use lib (split(/:/, $ENV{GITPERLLIB}));
>>>
>>> splits off the drive letter from the rest of the path. Obviously, this
>>> fails to Do The Right Thing, and simply points to Yet Another Portability
>>> Problem with Git's reliance on Unix scripting.
>>
>> In our C code, we have "#define PATH_SEP ';'", and encourage our
>> code to be careful and use it.  Is there something similar for Perl
>> scripts, I wonder.
>>
>> I notice that t/{t0202,t9000,t9700}/test.pl share the same
>> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
>> the non-platform convention to accomodate the use of split(/:/)
>> certainly is a workaround, but it does feel dirty.
>>
>> It is hard to imagine that we were the first people who wants to
>> split the value of a variable into a list, where the value is a list
>> of paths, concatenated into a single string with a delimiter that
>> may be platform specific.  I wonder if we are going against a best
>> practice established in the Perl world, simply because we don't know
>> about it (i.e. basically, it would say "don't split at a colon
>> because not all world is Unix; use $this_module instead", similar to
>> "don't split at a slash, use File::Spec instead to extract path
>> components").
>>
>
> I thought there was a way to do this in File::Spec, but that's only
> for splitting regular paths, and not for splitting a list of paths
> separated by ":" or ";"
>
> We probably should find a better solution to allow this to work with
> windows style paths...? I know that python provides os.pathsep, but I
> haven't seen an equivalent for perl yet.
>
> The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
> maybe we should be using this?

I was testing this recently on the Perl included with Git for Windows
and it returns : for the path separator even though it's on Windows,
so I don't think that would work. The Perl in Git for Windows seems to
want UNIX-style inputs (something Dscho seemed to allude to in his
response earlier.). I'm not sure why it's that way, but he probably
knows.

Bryan

(Pardon my previous blank message; Gmail fail.)

>
> Thanks,
> Jake
>
> [1] https://perldoc.perl.org/Env.html
> [2] https://perldoc.perl.org/Config.html

Re: [PATCH 0/6] Create Git/Packet.pm

[Bryan Turner]

On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller  wrote:
> On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano  wrote:
>> Johannes Schindelin  writes:
>>
>>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
>>> the line
>>>
>>>   use lib (split(/:/, $ENV{GITPERLLIB}));
>>>
>>> splits off the drive letter from the rest of the path. Obviously, this
>>> fails to Do The Right Thing, and simply points to Yet Another Portability
>>> Problem with Git's reliance on Unix scripting.
>>
>> In our C code, we have "#define PATH_SEP ';'", and encourage our
>> code to be careful and use it.  Is there something similar for Perl
>> scripts, I wonder.
>>
>> I notice that t/{t0202,t9000,t9700}/test.pl share the same
>> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
>> the non-platform convention to accomodate the use of split(/:/)
>> certainly is a workaround, but it does feel dirty.
>>
>> It is hard to imagine that we were the first people who wants to
>> split the value of a variable into a list, where the value is a list
>> of paths, concatenated into a single string with a delimiter that
>> may be platform specific.  I wonder if we are going against a best
>> practice established in the Perl world, simply because we don't know
>> about it (i.e. basically, it would say "don't split at a colon
>> because not all world is Unix; use $this_module instead", similar to
>> "don't split at a slash, use File::Spec instead to extract path
>> components").
>>
>
> I thought there was a way to do this in File::Spec, but that's only
> for splitting regular paths, and not for splitting a list of paths
> separated by ":" or ";"
>
> We probably should find a better solution to allow this to work with
> windows style paths...? I know that python provides os.pathsep, but I
> haven't seen an equivalent for perl yet.
>
> The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
> maybe we should be using this?
>
> Thanks,
> Jake
>
> [1] https://perldoc.perl.org/Env.html
> [2] https://perldoc.perl.org/Config.html

Re: [PATCH 0/6] Create Git/Packet.pm

[Jacob Keller]

On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano  wrote:
> Johannes Schindelin  writes:
>
>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
>> the line
>>
>>   use lib (split(/:/, $ENV{GITPERLLIB}));
>>
>> splits off the drive letter from the rest of the path. Obviously, this
>> fails to Do The Right Thing, and simply points to Yet Another Portability
>> Problem with Git's reliance on Unix scripting.
>
> In our C code, we have "#define PATH_SEP ';'", and encourage our
> code to be careful and use it.  Is there something similar for Perl
> scripts, I wonder.
>
> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> the non-platform convention to accomodate the use of split(/:/)
> certainly is a workaround, but it does feel dirty.
>
> It is hard to imagine that we were the first people who wants to
> split the value of a variable into a list, where the value is a list
> of paths, concatenated into a single string with a delimiter that
> may be platform specific.  I wonder if we are going against a best
> practice established in the Perl world, simply because we don't know
> about it (i.e. basically, it would say "don't split at a colon
> because not all world is Unix; use $this_module instead", similar to
> "don't split at a slash, use File::Spec instead to extract path
> components").
>

I thought there was a way to do this in File::Spec, but that's only
for splitting regular paths, and not for splitting a list of paths
separated by ":" or ";"

We probably should find a better solution to allow this to work with
windows style paths...? I know that python provides os.pathsep, but I
haven't seen an equivalent for perl yet.

The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
maybe we should be using this?

Thanks,
Jake

[1] https://perldoc.perl.org/Env.html
[2] https://perldoc.perl.org/Config.html

Re: [PATCH 0/6] Create Git/Packet.pm

[Junio C Hamano]

Johannes Schindelin  writes:

> Note that the correct blib path starts with `C:\BuildAgent\_work` and
> the line
>
>   use lib (split(/:/, $ENV{GITPERLLIB}));
>
> splits off the drive letter from the rest of the path. Obviously, this
> fails to Do The Right Thing, and simply points to Yet Another Portability
> Problem with Git's reliance on Unix scripting.

In our C code, we have "#define PATH_SEP ';'", and encourage our
code to be careful and use it.  Is there something similar for Perl
scripts, I wonder.

I notice that t/{t0202,t9000,t9700}/test.pl share the same
split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
the non-platform convention to accomodate the use of split(/:/)
certainly is a workaround, but it does feel dirty.

It is hard to imagine that we were the first people who wants to
split the value of a variable into a list, where the value is a list
of paths, concatenated into a single string with a delimiter that
may be platform specific.  I wonder if we are going against a best
practice established in the Perl world, simply because we don't know
about it (i.e. basically, it would say "don't split at a colon
because not all world is Unix; use $this_module instead", similar to
"don't split at a slash, use File::Spec instead to extract path
components").

Re: [PATCH 0/6] Create Git/Packet.pm

[Johannes Schindelin]

Hi Christian,

On Thu, 19 Oct 2017, Christian Couder wrote:

>   Add Git/Packet.pm from parts of t0021/rot13-filter.pl
> 
>  perl/Git/Packet.pm  | 118 
> 

This change, together with forcing t0021/rot13-filter.pl to use
Git/Packet.pm, breaks the test suite on Windows:

https://travis-ci.org/git/git/jobs/292461846

There are actually two problems, one of which is fixed by

-- snip --
diff --git a/perl/Makefile b/perl/Makefile
index 15d96fcc7a5..f657de20e39 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -30,6 +30,7 @@ instdir_SQ = $(subst ','\'',$(prefix)/lib)
 modules += Git
 modules += Git/I18N
 modules += Git/IndexInfo
+modules += Git/Packet
 modules += Git/SVN
 modules += Git/SVN/Memoize/YAML
 modules += Git/SVN/Fetcher
-- snap --

You will want to pick this up in the next iteration. (You simply did not
notice because you did not build with NO_PERL_MAKEMAKER.)

However, that *still* does not fix the test for me: note how in the
verbose output recorded in Travis (see the link above), Perl fails to find
the Perl modules and then says:

Can't locate Git/Packet.pm in @INC (you may need to install the Git::Packet 
module) (@INC contains: C \BuildAgent\_work\5\s\perl\blib\lib;C 
\BuildAgent\_work\5\s\perl\blib\arch\auto\Git /usr/lib/perl5/site_perl [..]

Note that the correct blib path starts with `C:\BuildAgent\_work` and
the line

use lib (split(/:/, $ENV{GITPERLLIB}));

splits off the drive letter from the rest of the path. Obviously, this
fails to Do The Right Thing, and simply points to Yet Another Portability
Problem with Git's reliance on Unix scripting.

But why is it a Windows path in the first place? We take pains at using
only Unix-style paths in Git's test suite after all.

Well, this one is easy. We call git.exe, which is a pure Win32 executable
(i.e. it *wants* Windows paths, even in the environment passed to it) and
git.exe in turn calls Perl to interpret rot13-filter.pl. So on the way to
git.exe, GITPERLLIB is converted by the MSYS2 runtime into a Windows-style
path list. And then it is not converted back when we call Perl.

As a workaround, I used a trick to exclude GITPERLLIB from being converted
by MSYS2: setting the environment variable MSYS2_ENV_CONV_EXCL=GITPERLLIB
"fixed" the test for me (with above patch thrown in). I also set the test
in the Visual Studio Team Services build definition that runs those tests
triggered by Travis.

If your patch makes it into Git's `master`, we may have to hardcode that
MSYS2_ENV_CONV_EXCL=GITPERLLIB (or augment any existing
MSYS2_ENV_CONV_EXCL), so that other Windows developers do not have to
stumble over the same thing and then spend 3 hours to debug it.

Ciao,
Dscho

Re: [PATCH 0/6] Create Git/Packet.pm

[Philip Oakley]

Hi Junio,

From: "Junio C Hamano" 

Christian Couder  writes:


Goal



Totally offtopic, but is it only me who finds these "section
headers" in cover letters from some people irritating and/or
jarring?


Personally I find that, for significant patch series, that clearly breaking 
out these distinct sections is of advantage. At this stage (the very first 
patch 0/n) there is no specific conversation, so the subject line is a short 
'hello' to the topic, and then the contributor is (it is to be hoped) 
setting out their proposal in a clear manner.


So I do like these headings for larger series, though there is some 
judgement to be made as to when the subject line alone is sufficient.


As a separate follow on, one thing that does annoy me is that in subsequent 
versions of the various patch series, folk tend to drop all explanation of 
why the series is of any relevance, leaving just the 'changed since last 
time' part. This means that new readers who try and pick up / review / 
contribute to a series later on in its development are not told the purpose. 
When the list is active it can, accidentally, do a disservice to the 
potential contributors who may feel that only core contributors are able to 
contribute.


Whether this series needed a Goal heading is separate discussion ;-)


..  It perhaps is because I view the cover letter more as a
part of conversation, not a presentation.  And when you walk up to
somebody and start a conversation, you do not declare section
headers ;-)

Saying "I want to be able to do these things in the future, and here
is to prepare for that future" at the beginning nevertheless is a
good thing.  It gives us readers an overall vision we can agree to
(or be against, or offer alternatives) and sets expectations on what
the series would do and where it stops and leaves the remainder to
follow-up work.


Packet related functions in Perl can be useful to write new filters or
to debug or test existing filters. So instead of having them in
t0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm
module.


I left some comments on individual patches to point out places that
may need improvements.  I agree with the overall direction.

Thanks for starting this topic.

--
Philip 

Re: [PATCH 0/6] Create Git/Packet.pm

[Junio C Hamano]

Christian Couder  writes:

> Goal
> 

Totally offtopic, but is it only me who finds these "section
headers" in cover letters from some people irritating and/or
jarring?  It perhaps is because I view the cover letter more as a
part of conversation, not a presentation.  And when you walk up to
somebody and start a conversation, you do not declare section
headers ;-)  

Saying "I want to be able to do these things in the future, and here
is to prepare for that future" at the beginning nevertheless is a
good thing.  It gives us readers an overall vision we can agree to
(or be against, or offer alternatives) and sets expectations on what
the series would do and where it stops and leaves the remainder to
follow-up work.

> Packet related functions in Perl can be useful to write new filters or
> to debug or test existing filters. So instead of having them in
> t0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm
> module.

I left some comments on individual patches to point out places that
may need improvements.  I agree with the overall direction.

Thanks for starting this topic.

[PATCH 0/6] Create Git/Packet.pm

[Christian Couder]

Goal


Packet related functions in Perl can be useful to write new filters or
to debug or test existing filters. So instead of having them in
t0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm
module.

Links
~

This patch series has been extracted from previous "Add initial
experimental external ODB support" patch series.

Version 1, 2, 3, 4, 5 and 6 of this previous series are on the mailing
list here:

https://public-inbox.org/git/20160613085546.11784-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20160628181933.24620-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20161130210420.15982-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20170620075523.26961-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20170803091926.1755-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20170916080731.13925-1-chrisc...@tuxfamily.org/

They are also available in the following branches:

https://github.com/chriscool/git/commits/gl-external-odb12
https://github.com/chriscool/git/commits/gl-external-odb22
https://github.com/chriscool/git/commits/gl-external-odb61
https://github.com/chriscool/git/commits/gl-external-odb239
https://github.com/chriscool/git/commits/gl-external-odb373
https://github.com/chriscool/git/commits/gl-external-odb411


Christian Couder (6):
  t0021/rot13-filter: refactor packet reading functions
  t0021/rot13-filter: improve 'if .. elsif .. else' style
  t0021/rot13-filter: improve error message
  t0021/rot13-filter: add packet_initialize()
  t0021/rot13-filter: add capability functions
  Add Git/Packet.pm from parts of t0021/rot13-filter.pl

 perl/Git/Packet.pm  | 118 
 t/t0021/rot13-filter.pl | 110 +---
 2 files changed, 140 insertions(+), 88 deletions(-)
 create mode 100644 perl/Git/Packet.pm

-- 
2.15.0.rc1.106.g7e97f58a41