From: David Aguilar <davvid@gmail.com>
To: John Keeping <john@keeping.me.uk>
Cc: Junio C Hamano <gitster@pobox.com>,
git@vger.kernel.org, Johannes Sixt <j.sixt@viscovery.net>,
Sitaram Chamarty <sitaramc@gmail.com>,
Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks
Date: Sun, 24 Mar 2013 14:29:40 -0700 [thread overview]
Message-ID: <CAJDDKr7Uz44TQ8y2jpjhNadWUCD5Mo=GLdaLLh99eENARQSwcw@mail.gmail.com> (raw)
In-Reply-To: <20130324123620.GA2286@serenity.lan>
On Sun, Mar 24, 2013 at 5:36 AM, John Keeping <john@keeping.me.uk> wrote:
> On Sat, Mar 23, 2013 at 10:19:36PM -0700, Junio C Hamano wrote:
>> > In the longer term, difftool probably needs to learn to warn the user
>> > instead of overwrite any changes that have been made to the working tree
>> > file.
>>
>> Questionable.
>>
>> Admittedly I do not use difftool myself, and I have long assumed
>> that difftool users are using the tools to _view_ the changes, but
>> apparently some of the tools let the user muck with what is shown,
>> and also apparently people seem to like the fact that they can make
>> changes. So I've led to believe the "update in difftool, take the
>> change back to working tree, either by making symbolic links or
>> copying them back" behaviour was a _feature_.
>
> Yes it is. I think my explanation wasn't clear enough here.
>
> What currently happens is that after the user's tool has finished
> running the working tree file and temporary file are compared and if
> they are different then the temporary file is copied over the working
> tree file.
>
> This is good if the user has edited the temporary file, but what if they
> edit they working tree file while using the tool to examine the
> differences? I think we need to at the very least look at the mtime of
> the files and refuse to copy over the temporary file if that of the
> working tree file is newer.
>
> Obviously none of this matters if we can use symlinks, but in the
> non-symlink case I think a user might find it surprising if the
> (unmodified) file used by their diff tool were suddenly copied over the
> working tree wiping out the changes they have just made.
Thanks, this adds a little more safety to the operation, which is good.
The downside is that it's a performance hit since we end up running
an additional hash-object on every worktree file.
I would definitely choose safety/correctness in this situation.
This makes me wonder whether the modifiable mode should be made
more explicit, either in the documentation or via a flag.
Imagine if --dir-diff also honored --edit and --no-edit flags.
Right now --edit is the default. If we had foreseen these various
edge cases and unintended copy-backs then we may have initially
chosen --no-edit as the default, but that's not really my point.
What I'm thinking is that it might be good for the tool to
learn --edit/--no-edit so that the symlink/copy-back heuristic
can be documented alongside that option. Users can then know
what to expect when using this mode. --no-edit would also be
faster since it can avoid all these extra steps.
It could also learn "difftool.dirDiffEditable" to control the
default, which would eliminate the pain in needing to supply
the flag on every invocation.
What do you think about officially supporting a read-only mode?
--
David
next prev parent reply other threads:[~2013-03-24 21:30 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-21 4:03 [PATCH v3 1/4] difftool: silence uninitialized variable warning David Aguilar
2013-02-21 4:03 ` [PATCH 2/4] t7800: update copyright notice David Aguilar
2013-02-21 4:03 ` [PATCH v3 3/4] t7800: modernize tests David Aguilar
2013-02-21 4:03 ` [PATCH v3 4/4] t7800: "defaults" is no longer a builtin tool name David Aguilar
2013-02-21 4:55 ` Junio C Hamano
2013-02-21 5:00 ` Junio C Hamano
2013-02-21 23:31 ` David Aguilar
2013-03-20 9:48 ` [PATCH v3 3/4] t7800: modernize tests Johannes Sixt
2013-03-20 22:59 ` David Aguilar
2013-03-21 7:41 ` Johannes Sixt
2013-03-22 7:13 ` Johannes Sixt
2013-03-22 10:00 ` John Keeping
2013-03-22 11:14 ` Johannes Sixt
2013-03-22 11:53 ` John Keeping
2013-03-22 19:36 ` [PATCH 0/3] Improve difftool --dir-diff tests John Keeping
2013-03-22 19:36 ` [PATCH 1/3] t7800: don't hide grep output John Keeping
2013-03-22 22:32 ` Johannes Sixt
2013-03-22 22:45 ` Junio C Hamano
2013-03-22 19:36 ` [PATCH 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping
2013-03-22 22:27 ` Johannes Sixt
2013-03-22 22:53 ` Junio C Hamano
2013-03-22 23:05 ` John Keeping
2013-03-23 3:24 ` David Aguilar
2013-03-22 19:36 ` [PATCH 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping
2013-03-22 21:05 ` [PATCH 3/3 v2] " John Keeping
2013-03-23 13:31 ` [PATCH v2 0/3] difftool --dir-diff test improvements John Keeping
2013-03-23 13:31 ` [PATCH v2 1/3] t7800: don't hide grep output John Keeping
2013-03-23 13:31 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks John Keeping
2013-03-24 5:19 ` Junio C Hamano
2013-03-24 12:36 ` John Keeping
2013-03-24 13:31 ` Matt McClure
2013-03-24 15:15 ` John Keeping
2013-03-25 7:41 ` Johannes Sixt
2013-03-25 10:42 ` John Keeping
2013-03-25 21:44 ` [PATCH v2] difftool: don't overwrite modified files John Keeping
2013-03-26 8:38 ` Johannes Sixt
2013-03-26 8:47 ` Johannes Sixt
2013-03-26 9:31 ` John Keeping
2013-03-26 9:53 ` Johannes Sixt
2013-03-26 19:34 ` John Keeping
2013-03-26 20:52 ` Matt McClure
2013-03-26 21:01 ` John Keeping
2013-03-25 16:15 ` [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks Junio C Hamano
2013-03-24 21:29 ` David Aguilar [this message]
2013-03-25 10:57 ` John Keeping
2013-03-25 14:54 ` Junio C Hamano
2013-03-24 13:24 ` Matt McClure
2013-03-24 6:20 ` Eric Sunshine
2013-03-23 13:31 ` [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks John Keeping
2013-03-25 7:26 ` Johannes Sixt
2013-03-25 10:35 ` John Keeping
2013-03-25 10:59 ` Johannes Sixt
2013-03-25 11:02 ` John Keeping
2013-03-25 21:50 ` Junio C Hamano
2013-03-26 9:22 ` John Keeping
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJDDKr7Uz44TQ8y2jpjhNadWUCD5Mo=GLdaLLh99eENARQSwcw@mail.gmail.com' \
--to=davvid@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j.sixt@viscovery.net \
--cc=john@keeping.me.uk \
--cc=jrnieder@gmail.com \
--cc=sitaramc@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).