On 12/2/2019 7:18 PM, Denton Liu wrote:
Hi Ben,
Thanks for the contribution!
Subject: Python3 support for t9800 tests. Basic P4/Python3 support
In git.git, the convention for commit subjects is to use
"<area>: <summary>". Perhaps something like, "git-p4: support Python 3"?
Although I doubt this patch should remain as is... More below.
I didn't realize the email message from gitgitgadget was going to be the
commit message, I thought it was the PR message. I'll work on changing
that!
On Mon, Dec 02, 2019 at 07:02:16PM +0000, Ben Keene via GitGitGadget wrote:
From: Ben Keene <seraphire@xxxxxxxxx>
It would be nice to have a bit more information about what this patch
does. Could you please fill this in with some more details about the
whats and, more importantly, the _whys_ of your change?
Sure, I'll add more detail.
Signed-off-by: Ben Keene <seraphire@xxxxxxxxx>
---
git-p4.py | 825 +++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 628 insertions(+), 197 deletions(-)
This is a very big change to be done in one patch. Could you please
split this into multiple smaller patches that each do one logical
change? For example, you could have the following series of changes:
1. git-p4: use p4.exe if on Windows
2. git-p4: introduce encoding helper functions # this is to
introduce the as_string(), as_bytes(), etc. functions
3. git-p4: start using the encoding helper functions
...
This was just an example and you don't have to follow those literally. I
just wanted to give you an idea of what I meant.
You can see Documentation/SubmittingPatches#separate-commits for more
information.
Thanks,
Denton
So my last question would be, should I open a different PR on
gitgitgadget? I can cherry-pick my changes into another branch and
restart my submission?
