All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mergetool merge/skip/abort
@ 2009-01-21 14:37 Caleb Cushing
  2009-01-21 16:17 ` Caleb Cushing
  2009-01-21 17:04 ` Charles Bailey
  0 siblings, 2 replies; 17+ messages in thread
From: Caleb Cushing @ 2009-01-21 14:37 UTC (permalink / raw)
  To: git

There are some files that I can't merge with git, and sometimes you
just want to finish merging later or move on to the next file and come
back later. My patch allows you to quit mergetool without ctrl-c, or
move on to the next file or merge the this one. pretty simple and I
think will be useful for a lot of people.


>From b647762ad179cdaaf9f844671fdf26074563b366 Mon Sep 17 00:00:00 2001
From: Caleb Cushing <xenoterracide@gmail.com>
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 the merge

---
 git-mergetool.sh |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 00e1337..43d2a9e 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -177,11 +177,27 @@ 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
-    fi
+               while true; do
+               printf "Use (m)erge file or (s)skip 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
+       case "$merge_tool" in
        kdiff3)
            if base_present ; then
                ("$merge_tool_path" --auto --L1 "$MERGED (Base)" --L2
"$MERGED (Local)" --L3 "$MERGED (Remote)" \
--
1.6.1






-- 
Caleb Cushing

http://xenoterracide.blogspot.com

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-21 14:37 [PATCH] mergetool merge/skip/abort Caleb Cushing
@ 2009-01-21 16:17 ` Caleb Cushing
  2009-01-21 16:33   ` Johannes Schindelin
  2009-01-21 18:49   ` Markus Heidelberg
  2009-01-21 17:04 ` Charles Bailey
  1 sibling, 2 replies; 17+ messages in thread
From: Caleb Cushing @ 2009-01-21 16:17 UTC (permalink / raw)
  To: git

just a typo fix in the patch

>From 29c2873861a3aec8304529735307385e9e5c248a Mon Sep 17 00:00:00 2001
From: Caleb Cushing <xenoterracide@gmail.com>
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 the merge

---
 git-mergetool.sh |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 00e1337..ae94300 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -177,11 +177,27 @@ 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
-    fi
+               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
+       case "$merge_tool" in
        kdiff3)
            if base_present ; then
                ("$merge_tool_path" --auto --L1 "$MERGED (Base)" --L2
"$MERGED (Local)" --L3 "$MERGED (Remote)" \
--
1.6.1

-- 
Caleb Cushing

http://xenoterracide.blogspot.com

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-21 16:17 ` Caleb Cushing
@ 2009-01-21 16:33   ` Johannes Schindelin
  2009-01-21 18:49   ` Markus Heidelberg
  1 sibling, 0 replies; 17+ messages in thread
From: Johannes Schindelin @ 2009-01-21 16:33 UTC (permalink / raw)
  To: Caleb Cushing; +Cc: git

Hi,

On Wed, 21 Jan 2009, Caleb Cushing wrote:

> just a typo fix in the patch
> 
> >From 29c2873861a3aec8304529735307385e9e5c248a Mon Sep 17 00:00:00 2001
> From: Caleb Cushing <xenoterracide@gmail.com>
> 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 the merge
> 
> ---

If you look at other patch submissions, you will find that they all do it 
differently.  You will never see a "Date:" or "Subject:" line.  You will 
see a much shorter commit subject.  And a more verbose commit message.

Sometimes, you will see that a commit message is not quite informative, or 
does not explain the _motivation_ why it should be a good idea to do what 
the patch does, and me saying so very directly (but never meaning to 
offend).

Hth,
Dscho

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-21 14:37 [PATCH] mergetool merge/skip/abort Caleb Cushing
  2009-01-21 16:17 ` Caleb Cushing
@ 2009-01-21 17:04 ` Charles Bailey
  2009-01-22 14:17   ` Caleb Cushing
  1 sibling, 1 reply; 17+ messages in thread
From: Charles Bailey @ 2009-01-21 17:04 UTC (permalink / raw)
  To: Caleb Cushing; +Cc: git

On Wed, Jan 21, 2009 at 09:37:20AM -0500, Caleb Cushing wrote:
> ---
>  git-mergetool.sh |   24 ++++++++++++++++++++----
>  1 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 00e1337..43d2a9e 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -177,11 +177,27 @@ 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
> -    fi
> +               while true; do
> +               printf "Use (m)erge file or (s)skip 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

This looks to me like no merge will happen if --no-prompt/-y or
mergetool.prompt is set to false. Have you tested with this option or
have I misread?

Also, I think you've lost some tabs. Mergetool does have some
inconsistent tabbing but they way I've been aiming towards (which
matches most, but not all of git-mergetool.sh) is to use tabs == 8
spaces for indents but to indent each level by 4 spaces. e.g. three
levels of indent is one tab plus four spaces.

It might be quite nice to offer the option of directly using an 'ours'
or 'theirs' as an alternative to skip for binary files. A bit like
symlinks are handled in mergetool.

Charles.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-21 16:17 ` Caleb Cushing
  2009-01-21 16:33   ` Johannes Schindelin
@ 2009-01-21 18:49   ` Markus Heidelberg
  2009-01-22  5:07     ` Caleb Cushing
  1 sibling, 1 reply; 17+ messages in thread
From: Markus Heidelberg @ 2009-01-21 18:49 UTC (permalink / raw)
  To: Caleb Cushing; +Cc: git

Caleb Cushing, 21.01.2009:
>  git-mergetool.sh |   24 ++++++++++++++++++++----
>  1 files changed, 20 insertions(+), 4 deletions(-)

> +               case "$ans" in
> +                       [mM]*)
> +                       break

I'd like to keep (additionally) the behaviour, that the merge starts
with just pressing <Enter>. Because what you mostly want to do, when
using git-mergetool, is actually merging.

> -    case "$merge_tool" in
> +       case "$merge_tool" in
>         kdiff3)

This doesn't seem right.

Markus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-21 18:49   ` Markus Heidelberg
@ 2009-01-22  5:07     ` Caleb Cushing
  0 siblings, 0 replies; 17+ messages in thread
From: Caleb Cushing @ 2009-01-22  5:07 UTC (permalink / raw)
  To: markus.heidelberg; +Cc: git

> This looks to me like no merge will happen if --no-prompt/-y or
>  mergetool.prompt is set to false. Have you tested with this option or
>  have I misread?

sorry haven't tested as I don't use that. will test in the morning...
if it doesn't work will try to get it working.

>  Also, I think you've lost some tabs. Mergetool does have some
>  inconsistent tabbing but they way I've been aiming towards (which
>  matches most, but not all of git-mergetool.sh) is to use tabs == 8
>  spaces for indents but to indent each level by 4 spaces. e.g. three
>  levels of indent is one tab plus four spaces.

thanks wasn't sure on the indentation, I set tabstop to 4 spaces in
vim so my tabs look like your spaces. I'll correct in the next case.

>  It might be quite nice to offer the option of directly using an 'ours'
>  or 'theirs' as an alternative to skip for binary files. A bit like
>  symlinks are handled in mergetool.

I could look into it... at the same time I don't have a good test case
so I'd rather leave it to someone else.


> I'd like to keep (additionally) the behaviour, that the merge starts
> with just pressing <Enter>. Because what you mostly want to do, when
> using git-mergetool, is actually merging.

I'd thought of that... and I'll see what I can do, although to me it
doesn't matter much.

> This doesn't seem right.

erm.. yeah... I'll fix it
-- 
Caleb Cushing

http://xenoterracide.blogspot.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-21 17:04 ` Charles Bailey
@ 2009-01-22 14:17   ` Caleb Cushing
  2009-01-22 14:22     ` Charles Bailey
  0 siblings, 1 reply; 17+ messages in thread
From: Caleb Cushing @ 2009-01-22 14:17 UTC (permalink / raw)
  To: Charles Bailey; +Cc: git

>From bf55fdd37f0fa4d0b3a10f43fa3d1815a6dbc6b3 Mon Sep 17 00:00:00 2001
From: Caleb Cushing <xenoterracide@gmail.com>
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(-)

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.6.1

> This looks to me like no merge will happen if --no-prompt/-y or
>  mergetool.prompt is set to false. Have you tested with this option or
>  have I misread?

just tested and it works fine... meaning it doesn't prompt which is
what it's supposed to do.

>  Also, I think you've lost some tabs. Mergetool does have some
>  inconsistent tabbing but they way I've been aiming towards (which
>  matches most, but not all of git-mergetool.sh) is to use tabs == 8
>  spaces for indents but to indent each level by 4 spaces. e.g. three
>  levels of indent is one tab plus four spaces.

mixing tabs and spaces == bad, I just realized I can't see half your
indents because I had tabstop=4 in my vimrc because I like indents at
4 spaces which is what you are doing through emulation. this is why I
generally just use tabs or spaces. files like fstab and .sql are my
few exceptions, both of which I do more on columns than indents.

> I'd like to keep (additionally) the behaviour, that the merge starts
> with just pressing <Enter>. Because what you mostly want to do, when
> using git-mergetool, is actually merging.

done, and maybe you... but generally I've got more skips than merges,
but I've got a corner case.
-- 
Caleb Cushing

http://xenoterracide.blogspot.com

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-22 14:17   ` Caleb Cushing
@ 2009-01-22 14:22     ` Charles Bailey
  2009-01-23 15:16       ` Caleb Cushing
  0 siblings, 1 reply; 17+ messages in thread
From: Charles Bailey @ 2009-01-22 14:22 UTC (permalink / raw)
  To: Caleb Cushing; +Cc: git

On Thu, Jan 22, 2009 at 09:17:39AM -0500, Caleb Cushing wrote:
> >  Also, I think you've lost some tabs. Mergetool does have some
> >  inconsistent tabbing but they way I've been aiming towards (which
> >  matches most, but not all of git-mergetool.sh) is to use tabs == 8
> >  spaces for indents but to indent each level by 4 spaces. e.g. three
> >  levels of indent is one tab plus four spaces.
> 
> mixing tabs and spaces == bad, I just realized I can't see half your
> indents because I had tabstop=4 in my vimrc because I like indents at
> 4 spaces which is what you are doing through emulation. this is why I
> generally just use tabs or spaces. files like fstab and .sql are my
> few exceptions, both of which I do more on columns than indents.

There are two conventions at work in git-mergetool.sh but the most
prevalent one (it was like that when I got here!) can be easily
maintained in vim with:

:set tabstop=8
:set softtabstop=4
:set shiftwidth=4

You'll never (well, almost never) know that tabs aren't 4 spaces.

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-22 14:22     ` Charles Bailey
@ 2009-01-23 15:16       ` Caleb Cushing
  2009-01-23 17:26         ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Caleb Cushing @ 2009-01-23 15:16 UTC (permalink / raw)
  To: Charles Bailey; +Cc: git

>  You'll never (well, almost never) know that tabs aren't 4 spaces.

no but I think people should be able to view indents at whatever they
want and still have it work, which is why I normally use tabs
exclusively, and set it to 4 space display, but if people wanted to
view at 2, 6, 8 spaces or whatever they could and the indents should
still look correct.

it doesn't really matter to each project there own.

so does my patch satisfy now? what's it take to get it included in the
next version of git?
-- 
Caleb Cushing

http://xenoterracide.blogspot.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-23 15:16       ` Caleb Cushing
@ 2009-01-23 17:26         ` Junio C Hamano
  2009-01-24 18:36           ` Caleb Cushing
  2009-01-26 22:58           ` Theodore Tso
  0 siblings, 2 replies; 17+ messages in thread
From: Junio C Hamano @ 2009-01-23 17:26 UTC (permalink / raw)
  To: Caleb Cushing; +Cc: Charles Bailey, git

Caleb Cushing <xenoterracide@gmail.com> 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)?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-23 17:26         ` Junio C Hamano
@ 2009-01-24 18:36           ` Caleb Cushing
  2009-01-24 21:45             ` Nanako Shiraishi
  2009-01-26 22:58           ` Theodore Tso
  1 sibling, 1 reply; 17+ messages in thread
From: Caleb Cushing @ 2009-01-24 18:36 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Charles Bailey, 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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-24 18:36           ` Caleb Cushing
@ 2009-01-24 21:45             ` Nanako Shiraishi
  2009-01-25  0:18               ` Caleb Cushing
  2009-01-25  5:20               ` Junio C Hamano
  0 siblings, 2 replies; 17+ messages in thread
From: Nanako Shiraishi @ 2009-01-24 21:45 UTC (permalink / raw)
  To: Caleb Cushing; +Cc: Junio C Hamano, Charles Bailey, git

Quoting Caleb Cushing <xenoterracide@gmail.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 <xenoterracide@gmail.com>
	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 <azure@iki.fi>, Theodore Tso
<tytso@mit.edu>' 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 <charles@hashpling.org>
	---
	 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 <heipei@hackvalue.de>
	---
	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/

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-24 21:45             ` Nanako Shiraishi
@ 2009-01-25  0:18               ` Caleb Cushing
  2009-01-25  5:20               ` Junio C Hamano
  1 sibling, 0 replies; 17+ messages in thread
From: Caleb Cushing @ 2009-01-25  0:18 UTC (permalink / raw)
  To: Nanako Shiraishi; +Cc: Junio C Hamano, Charles Bailey, git

>   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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-24 21:45             ` Nanako Shiraishi
  2009-01-25  0:18               ` Caleb Cushing
@ 2009-01-25  5:20               ` Junio C Hamano
  1 sibling, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2009-01-25  5:20 UTC (permalink / raw)
  To: Nanako Shiraishi; +Cc: Caleb Cushing, Charles Bailey, git

Nanako Shiraishi <nanako3@lavabit.com> writes:

>  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

I generally prefer the above style, but mergetool is not mine, and the
predominant style in it is:

        case xyzzy in
            frotz)
                do this
                ;;
            nitfol)
                do that
                ;;
        esac

Namely, case arms' labels are indented by 4 spaces from case/esac, and the
commands in each case arm are further indented by 4 spaces (including the
terminating double-semicolon).

It is always preferable to match the _local_ convention.  I'd expect a new
script added to git suite to match my preference (the one I showed you in
my comments to you that is used in git-am, which is what you suggested
above), but I'd expect a modification to mergetool to match the style
mergetool already uses.

IOW, Caleb's indentation style is fine.  The placement of double-semicolon
is not, though.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-23 17:26         ` Junio C Hamano
  2009-01-24 18:36           ` Caleb Cushing
@ 2009-01-26 22:58           ` Theodore Tso
  2009-01-27 22:09             ` Charles Bailey
  1 sibling, 1 reply; 17+ messages in thread
From: Theodore Tso @ 2009-01-26 22:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Caleb Cushing, Charles Bailey, git

On Fri, Jan 23, 2009 at 09:26:32AM -0800, Junio C Hamano wrote:
> Caleb Cushing <xenoterracide@gmail.com> 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 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-26 22:58           ` Theodore Tso
@ 2009-01-27 22:09             ` Charles Bailey
  2009-01-27 22:37               ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Charles Bailey @ 2009-01-27 22:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Theodore Tso, git

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/

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] mergetool merge/skip/abort
  2009-01-27 22:09             ` Charles Bailey
@ 2009-01-27 22:37               ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2009-01-27 22:37 UTC (permalink / raw)
  To: Charles Bailey; +Cc: Theodore Tso, git

Charles Bailey <charles@hashpling.org> writes:

> On Mon, Jan 26, 2009 at 05:58:35PM -0500, Theodore Tso wrote:
> ...
> 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.

Competence certainly counts to a certain extent, but volunteerism,
willingness, and enthusiasm count too.

Taste is sometimes a relative thing and we can make sure where we agree to
disagree on the list case by case basis.

Most importantly, anybody who will suffer when the tool breaks will be
much better person than I to look after it.  That is one of the largest
ingredient in the "trust" factor.

Thanks.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2009-01-27 22:38 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-01-21 14:37 [PATCH] mergetool merge/skip/abort Caleb Cushing
2009-01-21 16:17 ` Caleb Cushing
2009-01-21 16:33   ` Johannes Schindelin
2009-01-21 18:49   ` Markus Heidelberg
2009-01-22  5:07     ` Caleb Cushing
2009-01-21 17:04 ` Charles Bailey
2009-01-22 14:17   ` Caleb Cushing
2009-01-22 14:22     ` Charles Bailey
2009-01-23 15:16       ` Caleb Cushing
2009-01-23 17:26         ` Junio C Hamano
2009-01-24 18:36           ` Caleb Cushing
2009-01-24 21:45             ` Nanako Shiraishi
2009-01-25  0:18               ` Caleb Cushing
2009-01-25  5:20               ` Junio C Hamano
2009-01-26 22:58           ` Theodore Tso
2009-01-27 22:09             ` Charles Bailey
2009-01-27 22:37               ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.