From: Josef Ridky <hidden> Date: 2016-10-03 06:36:37
Hello,
I would like to request for implementing feature described in subject.
In several projects, we are using git mergetool for comparing files from different folders.
Unfortunately, when we have opened three files for comparing using meld tool (e.q. Old_version -- Result -- New_version),
we can see only name of temporary files created by mergetool in the labels (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
and users (and sometime even we) are confused, which of the files should they edit and save.
If you will be so kind and approve this feature request, you will help us to solve this unfortunate issue by the easiest way.
I have already prepared patch, which can be applied to resolve this request (see in attachment).
Best regards
Josef Ridky
Associate Software Engineer
Core Services Team
Red Hat Czech, s.r.o.
Hi Josef,
On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky [off-list ref] wrote:
In several projects, we are using git mergetool for comparing files from different folders.
Unfortunately, when we have opened three files for comparing using meld tool (e.q. Old_version -- Result -- New_version),
we can see only name of temporary files created by mergetool in the labels (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
and users (and sometime even we) are confused, which of the files should they edit and save.
`git mergetool` just creates temporary files (with some temporary
names) and calls `meld` (or `vimdiff`, etc) with the file names as
parameters. So why wouldn't you call `meld` with the file names you
want?
--
Mit freundlichen Grüßen,
Anatoly Borodin
From: Josef Ridky <hidden> Date: 2016-10-04 05:19:41
Hi Anatoly,
| Sent: Monday, October 3, 2016 5:18:44 PM
|
| Hi Josef,
|
|
| On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky [off-list ref] wrote:
| > In several projects, we are using git mergetool for comparing files from
| > different folders.
| > Unfortunately, when we have opened three files for comparing using meld
| > tool (e.q. Old_version -- Result -- New_version),
| > we can see only name of temporary files created by mergetool in the labels
| > (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
| > and users (and sometime even we) are confused, which of the files should
| > they edit and save.
|
| `git mergetool` just creates temporary files (with some temporary
| names) and calls `meld` (or `vimdiff`, etc) with the file names as
| parameters. So why wouldn't you call `meld` with the file names you
| want?
Because files, that we want, are temporary files created by git mergetool and we are not able to change their name.
Regards
Josef
From: Josef Ridky <hidden> Date: 2016-10-05 07:47:16
Hi,
I have just realize, that my attachment has been cut off from my previous message.
Below you can find patch with requested change.
Add support for user defined suffix part of name of temporary files
created by git mergetool
---
Documentation/git-mergetool.txt | 26 ++++++++++++++++++++-
git-mergetool.sh | 52 +++++++++++++++++++++++++++++++++++++----
2 files changed, 72 insertions(+), 6 deletions(-)
@@ -79,6 +79,30 @@ success of the resolution after the custom tool has exited. Prompt before each invocation of the merge resolution program to give the user a chance to skip the path.+--local=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (local) for merging. If not used or is equal with any+ other (remote|backup|base), default value is used.+ Default suffix is LOCAL.++--remote=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (remote) for merging. If not used or is equal with any+ other (local|backup|base), default value is used.+ Default suffix is REMOTE.++--backup=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (backup) for merging. If not used or is equal with any+ other (local|remote|base), default value is used.+ Default suffix is BACKUP.++--base=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (base) for merging. If not used or is equal with any+ other (local|remote|backup), default value is used.+ Default suffix is BASE.+ TEMPORARY FILES --------------- `git mergetool` creates `*.orig` backup files while resolving merges.
@@ -8,11 +8,18 @@# at the discretion of Junio C Hamano.#-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [--local=name] [--remote=name] [--backup=name] [--base=name] [file to merge] ...'SUBDIRECTORY_OK=YesNONGIT_OK=YesOPTIONS_SPEC=TOOL_MODE=merge++#optional name space convention+local_name=""+remote_name=""+base_name=""+backup_name=""+ .git-sh-setup .git-mergetool--lib
From: David Aguilar <hidden> Date: 2016-10-05 09:47:16
On Tue, Oct 04, 2016 at 01:18:47AM -0400, Josef Ridky wrote:
Hi Anatoly,
| Sent: Monday, October 3, 2016 5:18:44 PM
|
| Hi Josef,
|
|
| On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky [off-list ref] wrote:
| > In several projects, we are using git mergetool for comparing files from
| > different folders.
| > Unfortunately, when we have opened three files for comparing using meld
| > tool (e.q. Old_version -- Result -- New_version),
| > we can see only name of temporary files created by mergetool in the labels
| > (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
| > and users (and sometime even we) are confused, which of the files should
| > they edit and save.
|
| `git mergetool` just creates temporary files (with some temporary
| names) and calls `meld` (or `vimdiff`, etc) with the file names as
| parameters. So why wouldn't you call `meld` with the file names you
| want?
Because files, that we want, are temporary files created by
git mergetool and we are not able to change their name.
[I didn't see your original patch, but we actually prefer inline
patches in the email, as sent via `git send-email`.
Documentation/SubmittingPatches has more details.
Please also make sure to add a test to t/t7610-mergetool.sh
exercising any new features.]
Are you proposing support for config variables to control how
the temporary files are named?
e.g. something like "mergetool.strings.{local,remote,base}" for
overriding the hard-coded {LOCAL,REMOTE,BASE} strings?
I don't want to over-engineer it, but do you want to support
executing a command to get the name, or is having a replacement
sufficient?
Now I'm curious... if replacing the strings is sufficient, what
do you plan to call them? I can imagine maybe something like
OURS, and THEIRS might be helpful since it matches the
nomenclature already used by Git, e.g. "git merge -s ours".
Since these are temporary files, changing these names might not
be entirely out of the question. This might be a case where
using the same words as a related Git feature might help reduce
the mental burden of using mergetool. OURS and THEIRS are
probably the only names that fit that category, IMO.
BASE is already good enough (merge-base).
The downside of making it configurable is that it can confuse
users who use mergetool at someone else's desk where they've
named these strings to "catty", "wombat", and "jimbo". This
doesn't seem like the kind of place where we want to allow users
to be creative, but we do care about having a good default.
OURS and THEIRS are intuitive names, so switching existing users
to those would not have much downside IMO, and it's a little
less "I just merged a REMOTE branch" centric, which is good.
Do you think these names should be changed?
If so, did you have those names in mind, or something else
entirely?
cheers,
--
David
From: Josef Ridky <hidden> Date: 2016-10-05 10:19:46
Hi David,
thank you very much for your reply.
Today I realized, that my attachment has been cut off, so I sent it in the morning [1].
I believe, that most of answer can be find in my previous email from this morning.
Only change, that should be done by this request is add possibility to edit hard-coded suffix of temporary files.
I don't think, that change hard-coded suffix to switch between two hard-coded suffix of temporary files is suitable solution, but as well it's not bad solution.
Please look at the patch [1] and tell me, what do you think about this change.
[1] http://marc.info/?l=git&m=147565363609649&w=2
Regards
Josef
| Sent: Wednesday, October 5, 2016 11:47:06 AM
|
| On Tue, Oct 04, 2016 at 01:18:47AM -0400, Josef Ridky wrote:
| > Hi Anatoly,
| >
| >
| > | Sent: Monday, October 3, 2016 5:18:44 PM
| > |
| > | Hi Josef,
| > |
| > |
| > | On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky [off-list ref] wrote:
| > | > In several projects, we are using git mergetool for comparing files
| > | > from
| > | > different folders.
| > | > Unfortunately, when we have opened three files for comparing using
| > | > meld
| > | > tool (e.q. Old_version -- Result -- New_version),
| > | > we can see only name of temporary files created by mergetool in the
| > | > labels
| > | > (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
| > | > and users (and sometime even we) are confused, which of the files
| > | > should
| > | > they edit and save.
| > |
| > | `git mergetool` just creates temporary files (with some temporary
| > | names) and calls `meld` (or `vimdiff`, etc) with the file names as
| > | parameters. So why wouldn't you call `meld` with the file names you
| > | want?
| >
| >
| > Because files, that we want, are temporary files created by
| > git mergetool and we are not able to change their name.
|
| [I didn't see your original patch, but we actually prefer inline
| patches in the email, as sent via `git send-email`.
| Documentation/SubmittingPatches has more details.
|
| Please also make sure to add a test to t/t7610-mergetool.sh
| exercising any new features.]
|
| Are you proposing support for config variables to control how
| the temporary files are named?
|
| e.g. something like "mergetool.strings.{local,remote,base}" for
| overriding the hard-coded {LOCAL,REMOTE,BASE} strings?
|
| I don't want to over-engineer it, but do you want to support
| executing a command to get the name, or is having a replacement
| sufficient?
|
| Now I'm curious... if replacing the strings is sufficient, what
| do you plan to call them? I can imagine maybe something like
| OURS, and THEIRS might be helpful since it matches the
| nomenclature already used by Git, e.g. "git merge -s ours".
|
| Since these are temporary files, changing these names might not
| be entirely out of the question. This might be a case where
| using the same words as a related Git feature might help reduce
| the mental burden of using mergetool. OURS and THEIRS are
| probably the only names that fit that category, IMO.
| BASE is already good enough (merge-base).
|
| The downside of making it configurable is that it can confuse
| users who use mergetool at someone else's desk where they've
| named these strings to "catty", "wombat", and "jimbo". This
| doesn't seem like the kind of place where we want to allow users
| to be creative, but we do care about having a good default.
|
| OURS and THEIRS are intuitive names, so switching existing users
| to those would not have much downside IMO, and it's a little
| less "I just merged a REMOTE branch" centric, which is good.
|
| Do you think these names should be changed?
| If so, did you have those names in mind, or something else
| entirely?
|
| cheers,
| --
| David
|
From: Johannes Sixt <hidden> Date: 2016-10-05 21:04:45
Am 05.10.2016 um 09:47 schrieb Josef Ridky:
Add support for user defined suffix part of name of temporary files
created by git mergetool
Do I understand correctly that your users have problems to identify
which of the "_BASE_", "_LOCAL_", "_REMOTE_" and "_BACKUP_" files they
must edit? I agree that there is some room for improvement.
The goal is that you want the user to see the label, e.g., "_EDIT_THIS_"
instead of "_LOCAL_". Now you have to teach your users that they have to
pass --local=_EDIT_THIS_. Why don't you just teach your users to edit
the file labeled "_LOCAL_"?
Therefore, I think that your patch as written does not help to reduce
the confusion. It may be a building block for further improvement, but
if you stop here, it is pointless.
@@ -79,6 +79,30 @@ success of the resolution after the custom tool has exited. Prompt before each invocation of the merge resolution program to give the user a chance to skip the path.+--local=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (local) for merging. If not used or is equal with any+ other (remote|backup|base), default value is used.+ Default suffix is LOCAL.++--remote=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (remote) for merging. If not used or is equal with any+ other (local|backup|base), default value is used.+ Default suffix is REMOTE.++--backup=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (backup) for merging. If not used or is equal with any+ other (local|remote|base), default value is used.+ Default suffix is BACKUP.++--base=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (base) for merging. If not used or is equal with any+ other (local|remote|backup), default value is used.+ Default suffix is BASE.
From: Josef Ridky <hidden> Date: 2016-10-06 07:21:57
Hi Johannes,
thank you very much for this reply.
| Sent: Wednesday, October 5, 2016 11:04:17 PM
|
| Am 05.10.2016 um 09:47 schrieb Josef Ridky:
| > Add support for user defined suffix part of name of temporary files
| > created by git mergetool
|
| Do I understand correctly that your users have problems to identify
| which of the "_BASE_", "_LOCAL_", "_REMOTE_" and "_BACKUP_" files they
| must edit? I agree that there is some room for improvement.
|
| The goal is that you want the user to see the label, e.g., "_EDIT_THIS_"
| instead of "_LOCAL_". Now you have to teach your users that they have to
| pass --local=_EDIT_THIS_. Why don't you just teach your users to edit
| the file labeled "_LOCAL_"?
The goal of this request is not to teach our users, how to change labels using git mergetool.
The point is, that git mergetool is used by several of our packages in specific use-case.
If I understand well, git mergetool expecting comparing LOCAL changes against REMOTE status in git repository.
We use git mergetool to compare LOCAL OLD version of package with LOCAL NEW version of package.
In this use-case is really nonsense to label old version of package as LOCAL and new version of package as REMOTE.
This is point of confusion. This is the reason, why I am asking for the possibility to change the suffix of temporary files, which are shown to user for comparison.
And as well, users do not open these files themselves, these files are automatically opened by our packages, so users are not responsible for the name of temporary files. They just see the results.
|
| Therefore, I think that your patch as written does not help to reduce
| the confusion. It may be a building block for further improvement, but
| if you stop here, it is pointless.
|
I agree, that this patch is written as general as possible and can possibly bring more confusion than benefits.
I will make new version of this patch, where will be just simple switch between LOCAL/REMOTE and NEW/OLD.
| > SYNOPSIS
| > --------
| > [verse]
| > -'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [<file>...]
| > +'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>]
| > [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
| >
| > DESCRIPTION
| > -----------
| > @@ -79,6 +79,30 @@ success of the resolution after the custom tool has
| > exited.
| > Prompt before each invocation of the merge resolution program
| > to give the user a chance to skip the path.
| >
| > +--local=<name>::
| > + Use string from <name> as part of suffix of name of temporary
| > + file (local) for merging. If not used or is equal with any
| > + other (remote|backup|base), default value is used.
| > + Default suffix is LOCAL.
| > +
| > +--remote=<name>::
| > + Use string from <name> as part of suffix of name of temporary
| > + file (remote) for merging. If not used or is equal with any
| > + other (local|backup|base), default value is used.
| > + Default suffix is REMOTE.
| > +
| > +--backup=<name>::
| > + Use string from <name> as part of suffix of name of temporary
| > + file (backup) for merging. If not used or is equal with any
| > + other (local|remote|base), default value is used.
| > + Default suffix is BACKUP.
| > +
| > +--base=<name>::
| > + Use string from <name> as part of suffix of name of temporary
| > + file (base) for merging. If not used or is equal with any
| > + other (local|remote|backup), default value is used.
| > + Default suffix is BASE.
|
|
From: Josef Ridky <hidden> Date: 2016-10-06 13:09:32
This is the second of two variant for request to add option to change
suffix of name of temporary files generated by git mergetool. This
change is requested for cases, when is git mergetool used for local
comparison between two version of same package during package rebase.
Signed-off-by: Josef Ridky <redacted>
---
Documentation/git-mergetool.txt | 26 ++++++++++++++++++++++++-
git-mergetool.sh | 43 ++++++++++++++++++++++++++++++++++++-----
2 files changed, 63 insertions(+), 6 deletions(-)
@@ -79,6 +79,30 @@ success of the resolution after the custom tool has exited. Prompt before each invocation of the merge resolution program to give the user a chance to skip the path.+--local=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (local) for merging. If not used or is equal with any+ other (remote|backup|base), default value is used.+ Default suffix is LOCAL.++--remote=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (remote) for merging. If not used or is equal with any+ other (local|backup|base), default value is used.+ Default suffix is REMOTE.++--backup=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (backup) for merging. If not used or is equal with any+ other (local|remote|base), default value is used.+ Default suffix is BACKUP.++--base=<name>::+ Use string from <name> as part of suffix of name of temporary+ file (base) for merging. If not used or is equal with any+ other (local|remote|backup), default value is used.+ Default suffix is BASE.+ TEMPORARY FILES --------------- `git mergetool` creates `*.orig` backup files while resolving merges.
@@ -8,7 +8,7 @@# at the discretion of Junio C Hamano.#-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [--local=name] [--remote=name] [--backup=name] [--base=name] [file to merge] ...'SUBDIRECTORY_OK=YesNONGIT_OK=YesOPTIONS_SPEC=
@@ -16,6 +16,12 @@ TOOL_MODE=merge .git-sh-setup .git-mergetool--lib+# Can be changed by user+LOCAL_NAME='LOCAL'+BASE_NAME='BASE'+BACKUP_NAME='BACKUP'+REMOTE_NAME='REMOTE'+# Returns true if the mode reflects a symlink is_symlink(){test"$1"=120000
From: Josef Ridky <hidden> Date: 2016-10-06 13:39:07
This is the first of two variant for request to add option to change
suffix of name of temporary files generated by git mergetool. This
change is requested for cases, when is git mergetool used for local
comparision between two version of same package during package rebase.
Signed-off-by: Josef Ridky <redacted>
---
Documentation/git-mergetool.txt | 7 ++++++-
git-mergetool.sh | 23 ++++++++++++++++++-----
2 files changed, 24 insertions(+), 6 deletions(-)
@@ -79,6 +79,11 @@ success of the resolution after the custom tool has exited. Prompt before each invocation of the merge resolution program to give the user a chance to skip the path.+-l::+--local::+ Change suffix of name of temporary files generated by git+ mergetool from `_REMOTE_` and `_LOCAL_` to `_OLD_` and `_NEW_`.+ TEMPORARY FILES --------------- `git mergetool` creates `*.orig` backup files while resolving merges.
@@ -8,10 +8,11 @@# at the discretion of Junio C Hamano.#-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-l|--local] [file to merge] ...'SUBDIRECTORY_OK=YesNONGIT_OK=YesOPTIONS_SPEC=+LOCAL_MODE=falseTOOL_MODE=merge .git-sh-setup .git-mergetool--lib
From: David Aguilar <hidden> Date: 2016-10-13 05:13:37
On Thu, Oct 06, 2016 at 08:43:02AM -0400, Josef Ridky wrote:
This is the first of two variant for request to add option to change
suffix of name of temporary files generated by git mergetool. This
change is requested for cases, when is git mergetool used for local
comparision between two version of same package during package rebase.
Signed-off-by: Josef Ridky <redacted>
---
Documentation/git-mergetool.txt | 7 ++++++-
git-mergetool.sh | 23 ++++++++++++++++++-----
2 files changed, 24 insertions(+), 6 deletions(-)
While I do like that this variant only uses a single flag, I was
curious whether it would make sense for us to change our
defaults to OLD/NEW? I'm thinking "no" since it's totally legit
to merge "old" branches so I'll stop there.
What I really wanted to mention was...
If the patch does not update t/t7610-mergetool.sh then there is
no guarantee that my clumsy fingers won't break the new feature
in the future ;-) So please make sure to update the tests once
we decide on the final direction. It makes sense we wouldn't
want to update them just yet (in this patch) since this is still
RFC, but the final one should include it.
I'm still leaning towards environment variables personally,
and would have no qualms against taking a patch that teaches it
to support environment variables as long as it adds a test case
for the new feature.
Thanks for sticking with it, Josef!
cheers,
--
David