From: Junio C Hamano <hidden> Date: 2016-06-15 22:46:01
Caleb Cushing [off-list ref] writes:
so does my patch satisfy now? what's it take to get it included in the
next version of git?
I do not use mergetool myself so I generally do not pay attention to
patches on this tool, but I would want to pick up ones that people
involved in mergetool discussion can agree to be good patches.
There are a few mergetool updates in flight from various authors. How
does your submission compare with others' in both form/presentation and
clarity of logic (remember, I am not keeping track)?
I do not use mergetool myself so I generally do not pay attention to
patches on this tool, but I would want to pick up ones that people
involved in mergetool discussion can agree to be good patches.
yeah I can see that.
There are a few mergetool updates in flight from various authors. How
does your submission compare with others' in both form/presentation and
clarity of logic (remember, I am not keeping track)?
to be honest, a quick search of the past 2 months of patches didn't
show me any patches that do the same thing as mine, so I'm not sure
that comparing one feature to a different feature is good. I did try
to remain consistent and even improve consistency with existing UI,
and use the same/similar logic to existing. I'm not keeping track
either, just fixing my own problem.
--
Caleb Cushing
http://xenoterracide.blogspot.com
There are a few mergetool updates in flight from various authors. How
does your submission compare with others' in both form/presentation and
clarity of logic (remember, I am not keeping track)?
to be honest, a quick search of the past 2 months of patches didn't
show me any patches that do the same thing as mine, so I'm not sure
that comparing one feature to a different feature is good.
Junio never asked what your patch does. He didn't ask if it does
something similar to what other patches do, either.
Your 81bfc67a0901220617l22b5a8e4ma48bb069d67cae91@mail.gmail.com with
'Subject: Re: [PATCH] mergetool merge/skip/abort' that is sent to you and
'Cc: git@vger.kernel.org' starts its body with:
From bf55fdd37f0fa4d0b3a10f43fa3d1815a6dbc6b3 Mon Sep 17 00:00:00 2001
From: Caleb Cushing [off-list ref]
Date: Tue, 20 Jan 2009 11:33:30 -0500
Subject: [PATCH] mergetool merge/skip/abort
add functionality to skip merging a file or abort from mergetool
---
git-mergetool.sh | 20 ++++++++++++++++++--
1 files changed, 18 insertions(+), 2 deletions(-)
For comparison, 1232578668-2203-1-git-send-email-charles@hashpling.org from
Charles Bailey with 'Subject: [PATCH] mergetool: respect autocrlf by using
checkout-index', with 'Cc: Hannu Koivisto [off-list ref], Theodore Tso
[off-list ref]' starts its message body this way:
Previously, git mergetool used cat-file which does not perform git to
worktree conversion. This changes mergetool to use git checkout-index
instead which means that the temporary files used for mergetool use the
correct line endings for the platform.
Signed-off-by: Charles Bailey [off-list ref]
---
git-mergetool.sh | 14 +++++++++++---
t/t7610-mergetool.sh | 15 +++++++++++++--
2 files changed, 24 insertions(+), 5 deletions(-)
Another example is 1232702093-24313-1-git-send-email-heipei@hackvalue.de
from Johannes Gilger with 'Subject: [PATCHv2] git mergetool: Don't repeat
merge tool candidates', sent to Junio, Theodore and the mailing list. Here
is its message body:
git mergetool listed some candidates for mergetools twice, depending on
the environment.
Signed-off-by: Johannes Gilger [off-list ref]
---
The first patch had the fatal flaw that it listed nothing when DISPLAY
and EDITOR/VISUAL were unset, we fixed that.
The order in which merge-candidates appear is still exactly the same,
only duplicates have been stripped. The check for KDE_FULL_SESSION was
removed since kdiff3 was added as long as DISPLAY was set and we weren't
running gnome.
git-mergetool.sh | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
Let's try to answer the first question Junio asked you together.
Can you spot the differences? How do they compare?
1. You copy-and-pasted output from format-patch, and have the header
part in the message body. Charles and Johannes have moved them to the
Email header.
Their messages are in the form the tool used for patch acceptance
expects. Yours isn't, and forces Junio to manually edit your message
before handling it.
2. You have a two-line Subject: without any commit message. Both Charles
and Johannes describe what their patches are about on the Subject
succinctly in a single line, and they have what old behavior their
patches change, and how their patches do so in their commit
messages. They explained why it is good to apply their patches
well. You didn't.
Johannes Schindelin even pointed out this and the previous point when
you sent your first version but you seem to have ignored him.
3..You quoted other people's comments after the patch and explained that
you addressed the issues, but didn't include them in your Cc list.
Charles has Hanuu on his Cc list, and also Theodore (the original
author) who knows the best about the tool. Johannes also sent his
patch to people who gave him review comments.
They made efforts to make sure that their patches are seen by people
who helped refine thier patches and/or by people who knows the script
that you are modifying well. You didn't.
4. You didn't sign your patch.
Please see Documentation/SubmittingPatches.
About the second question from Junio on the contents of the patch, I can
guess some comments you may receive from him when he reads your patch,
based on review comments I received from him on another shell script
recently.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 00e1337..bd5711e 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -177,8 +177,24 @@ merge_file () {
describe_file "$local_mode" "local" "$LOCAL"
describe_file "$remote_mode" "remote" "$REMOTE"
if "$prompt" = true; then
- printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
- read ans
+ while true; do
+ printf "Use (m)erge file or (s)kip file, or (a)bort? (%s): " \
+ "$merge_tool"
+ read ans
+ case "$ans" in
+ [mM]*|"")
+ break
+ ;;
+ [sS]*)
+ cleanup_temp_files
+ return 0
+ ;;
+ [aA]*)
+ cleanup_temp_files
+ exit 0
+ ;;
+ esac
+ done
fi
case "$merge_tool" in
1. Your printf message is funny. You either
(1) Use $merge_tool to merge file, or
(2) Skip file, or
(3) Abort.
but your message makes it look like:
(1) Use $merge_tool to Merge file, or
(2) Use $merge_tool to Skip file, or
(3) Use $merge_tool to Abort.
2. patterns in case command start at the same column as case and esac,
and ";;" is at the same column as any other commands.
case "$ans" in
[mM]*|"")
break
;;
[Ss]*)
...
esac
For what it's worth, I like what your patch does. I use mergetool from
time to time and I can imagine that this new feature will be useful.
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
1. You copy-and-pasted output from format-patch, and have the header
part in the message body. Charles and Johannes have moved them to the
Email header.
Their messages are in the form the tool used for patch acceptance
expects. Yours isn't, and forces Junio to manually edit your message
before handling it.
I'll get to the rest later...
but I tried sending the patch via email as you said followed this...
verbatim except replacing user@ and p4ssw0rd with my credentials, and
I got an auth error back. currently I've no idea how I would send
stuff from gmail. and I reject inline patches in funtoo because I use
webmail and they are impossible for me to handle easily.
Submitting properly formatted patches via Gmail is simple now that
IMAP support is available. First, edit your ~/.gitconfig to specify your
account settings:
[imap]
folder = "[Gmail]/Drafts"
host = imaps://imap.gmail.com
user = user@gmail.com
pass = p4ssw0rd
port = 993
sslverify = false
Next, ensure that your Gmail settings are correct. In "Settings" the
"Use Unicode (UTF-8) encoding for outgoing messages" should be checked.
Once your commits are ready to send to the mailing list, run the following
command to send the patch emails to your Gmail Drafts folder.
$ git format-patch -M --stdout origin/master | git imap-send
--
Caleb Cushing
http://xenoterracide.blogspot.com
On Fri, Jan 23, 2009 at 09:26:32AM -0800, Junio C Hamano wrote:
Caleb Cushing [off-list ref] writes:
quoted
so does my patch satisfy now? what's it take to get it included in the
next version of git?
I do not use mergetool myself so I generally do not pay attention to
patches on this tool, but I would want to pick up ones that people
involved in mergetool discussion can agree to be good patches.
There are a few mergetool updates in flight from various authors. How
does your submission compare with others' in both form/presentation and
clarity of logic (remember, I am not keeping track)?
I was the original author of mergetool, and for a while I was the
person that was reviewing and managing the mergetool patches for
Junio. Unfortunately, in the last couple of months I just haven't had
the time keep up with the various mergetool proposed patch updates.
So maybe it's time for me to hand it off to someone who has the time
and interest in continuing to hack mergetool, and has the necessary
"good taste" and such that Junio would be willing to trust that person
to be the git mergetool patch wrangler?
- Ted
From: Charles Bailey <hidden> Date: 2016-06-15 22:46:02
On Mon, Jan 26, 2009 at 05:58:35PM -0500, Theodore Tso wrote:
I was the original author of mergetool, and for a while I was the
person that was reviewing and managing the mergetool patches for
Junio. Unfortunately, in the last couple of months I just haven't had
the time keep up with the various mergetool proposed patch updates.
So maybe it's time for me to hand it off to someone who has the time
and interest in continuing to hack mergetool, and has the necessary
"good taste" and such that Junio would be willing to trust that person
to be the git mergetool patch wrangler?
- Ted
A quick blame session has shown that after Ted I've probably touched
the next most number of lines of mergetool. It's a crude measure and
not necessarily a sign of competence, I admit.
Although not rolling in spare time, I feel I'd be able review
mergetool patches at roughly the rate that they tend to appear at the
moment.
Given the above, if I pass the "good taste" and "Junio trust" tests I
feel that I should offer my services as mergetool patch wrangler.
--
Charles Bailey
http://ccgi.hashpling.plus.com/blog/