Re: Git trademark status and policy

[David Aguilar]

On Tue, Sep 18, 2018 at 02:22:22PM -0400, Jeff King wrote:
> On Mon, Sep 17, 2018 at 11:25:31AM +0200, Christian Couder wrote:
> 
> > > (Also, to be clear, this is all _only_ about "Git Cola". The "git-cola"
> > > command is explicitly OK in the policy because that's how commands
> > > work).
> > 
> > I agree about "git-cola" though I wonder about "git-dag" as this is
> > another command used by the project that is more generic. For example
> > I could imagine that, if we wanted to provide a shortcut for `git log
> > --graph --decorate --oneline`, we might want to use `git dag`.
> > 
> > I guess we can still recommend to change it if possible, though we can
> > also acknowledge that, as our recommendation comes very late (too
> > late?), it is just a "weak" recommendation.
> 
> Yeah, I agree with you, though I think it is a separate issue. "git-dag"
> is explicitly OK in the trademark policy, and they are not using "Git
> Dag" in any recognizable way.
> 
> So I think there is no trademark issue, but "git-dag" is probably just
> not a great idea in general, because the namespace is open and it is
> likely to get stomped by some other project. Or git itself. Or it may
> even be annoying for users who have a "git dag" alias (on-disk commands
> always override aliases).
> 
> So I think we should generally recommend against such generic names
> during the naming phase. At this point, I'm not sure the pain of
> changing now is any less than the pain of changing later if and when
> there's a conflict.
> 
> I think I'm actually violently agreeing with you, but I wanted to make
> it clear. :) (And everything else in your email seemed sensible, too).
> 
> -Peff


Thanks for the recommendation.  I'm open to changing the name in a
future major release.  For users that already use the short "dag" name,
we can transition over to something else if it's relatively short and
sweet.

Maybe a better name would be "git-kcola" (a nod to gitk), or "git-vdag"
for "visual DAG"?  Any sugs?  I'm terrible at naming things, but I do
refrain from using additional "git-*" names beyond these two for the
project.  I kinda like "vdag" since it's easy to type, and nearby the
existing "dag" name.

There's also one more script, but it's never installed in the users's
$PATH and is more of an internal implementation detail.  Git Cola
includes a GIT_SEQUENCE_EDITOR-compatible "git-xbase" command that
provides a visual interactive rebase feature.  That command should
probably be renamed to "cola-git-seq-editor" to make that clearer, and
also to open up the possibility of installing it in bin/ in the future
since it is useful on its own.

The rationale for two commands is that worktree diff+commit and history
inspection are our two primary use-cases.  Everything else is provided
as a sub-command, "git cola rebase", "git cola stash", etc. so there's
not much pressure to add more top-level names, just these two.

Thoughts?
-- 
David

Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments

[David Aguilar]

On Wed, Oct 24, 2018 at 02:10:14PM +0900, Junio C Hamano wrote:
> Denton Liu  writes:
> 
> > Subject: Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments
> 
> Other people may point it out, but s/Accept/accept/.
> 
> > In line with how difftool accepts a -g/--[no-]gui option, make mergetool
> > accept the same option in order to use the `merge.guitool` variable to
> > find the default mergetool instead of `merge.tool`.
> > ...
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index 9a8b97a2a..e317ae003 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -350,17 +350,23 @@ guess_merge_tool () {
> >  }
> >  
> >  get_configured_merge_tool () {
> > -   # Diff mode first tries diff.tool and falls back to merge.tool.
> > -   # Merge mode only checks merge.tool
> > +   # If first argument is true, find the guitool instead
> > +   if [ "$1" = true ]
> 
> Don't use [] in our shell script (see Documentation/CodingGuidelines);
> say
> 
>   if "$1" = true
> 
> instead.

Perhaps a small typo dropped "test" -- I think it should be

if test "$1" = true


> > +   # Diff mode first tries diff.(gui)tool and falls back to 
> > merge.(gui)tool.
> > +   # Merge mode only checks merge.(gui)tool
> > if diff_mode
> > then
> > -   merge_tool=$(git config diff.tool || git config merge.tool)
> > +   merge_tool=$(git config diff.${gui_prefix}tool || git config 
> > merge.${gui_prefix}tool)
> > else
> > -   merge_tool=$(git config merge.tool)
> > +   merge_tool=$(git config merge.${gui_prefix}tool)
> > fi
> 
> In mode_ok shell function, we seem to be prepared to deal with a
> case where neither diff_mode nor merge_mode is true.  Should this
> codepath also be prepared to?  
> 
> This is not a comment to point out an issue with this patch.  It is
> a genuine question on the code after (or before for that matter)
> this patch is applied.
> 
> Thanks.


I think the code is okay.  mode_ok is setup that way to allow filtering
for the other mode's tools, but this code path is only concerned with
getting the default merge tool, which should only ever happen in one of
the two modes.

The bit about difftool falling back to mergetool's config is a
convenience so it does make sense to keep that for guitool as well.

The code after this part should handle merge_tool being empty just fine,
so once the `[ ... ]` vs `test` bit is updated, please feel free to add:

Acked-by: David Aguilar 


cheers,
-- 
David

Re: Git trademark status and policy

[David Aguilar]

Hi Peff,

On Thu, Feb 02, 2017 at 03:26:56AM +0100, Jeff King wrote:
> 
>   - Commands like "git-foo" (so you run "git foo") are generally OK.
> This is Git's well-known extension mechanism, so it doesn't really
> imply endorsement (on the other hand, you do not get to complain if
> you choose too generic a name and conflict with somebody else's use
> of the same git-foo name).
> 
>   - When "git-foo" exists, we've approved "Git Foo" as a matching
> project name, but we haven't decided on a general rule to cover this
> case.  The only example here is "Git LFS".

The "Git Cola" project[1][2] provides two fully-featured Git porcelains,
"git-cola" and "git-dag".  The DAG tool is never referred to as a
separate project, so shouldn't be a concern trademark wise.

The project dates back to 2007, while the "Git Cola" name dates back to 2008.
FTR, the name "Cola" is also a shout-out to Linux (comp.os.linux.announce).

Can we continue to use the name "Git Cola" going forward?


> So that's more or less where we're at now.  In my opinion, a few open
> questions are:
> 
>   3. Was granting "Git LFS" the right call? I think the project is a good
>  one and has worked well with the greater Git community. But I think
>  the name has implied some level of "officialness". We obviously
>  need to allow "git-lfs" as a name. But should the policy have said
>  "you can call this LFS, and the command is git-lfs, but don't say
>  'Git LFS'". I'm not sure.
> 
>  One option would have been to ask "git-foo" to prefer "Foo for Git"
>  instead of "Git Foo" in their branding (it's too late now for "Git
>  LFS", so this is a hypothetical question for future requests now).
> 
> -Peff

In my (biased) opinion, granting "Git LFS" was the right call.

As long as the project is clearly a separate, but primarily Git-centric,
project then it seems like the right approach to allow "Git Foo" for
open source projects that contribute positively to the Git ecosystem.

Lastly, due to time constraints, the Git Cola logo is a tweaked version
of the Git logo, which may convey a level of "officialness" that might
be unwanted.  We can work on a replacement if desired.

Part of keeping the logo/visual identity close to core Git is because
the tool was always meant to be strongly tied to Git's unique features.
It's probably the same reason why the git-lfs branding uses similar
orange/red palettes -- to convey cohesiveness.  I would prefer to keep
the visual identity as-is (including the logo).

Can we continue to use the derivative logo for the time being until a
replacement is produced?  Alternatively, can we keep the logo as-is?


cheers,

[1] https://git-cola.github.io/
[2] https://github.com/git-cola/git-cola
-- 
David

Re: Git for games working group

[David Aguilar]

On Fri, Sep 14, 2018 at 02:13:28PM -0700, John Austin wrote:
> Hey Taylor,
> 
> Great to have your support! I think LFS has done a great job so far
> solving the large file issue. I've been working myself on strategies
> for handling binary conflicts, and particularly how to do it in a
> git-friendly way (ie. avoiding as much centralization as possible and
> playing into the commit/branching model of git). I've got to a loose
> design that I like, but it'd be good to get some feedback, as well as
> hearing what other game devs would want in a binary conflict system.
> 
> - John

Hey John, thanks for LFS, and thanks to Taylor for bringing up this topic.

Regarding file locking, the gitolite docs are insightful:
http://gitolite.com/gitolite/locking/index.html

File locking is how P4 handles binary conflicts.  It's actually
conflict prevention -- the locks prevent users from stepping
on each other without needing to actually talk to each other.

(I've always believed that this is actually a social problem
 (not a technical one) that is best served by better communication,
 but there's no doubt that having a technical guard in place is useful
 in many scenarios.)

>From the POV of using Git as a P4 replacement, the locking support in
git-lfs seems like a fine solution to prevent binary conflicts.

https://github.com/git-lfs/git-lfs/wiki/File-Locking

Are there any missing features that would help improve LFS solution?


Locking is just one aspect of binary conflicts.

In a lock-free world, another aspect is tooling around dealing
with actual conflicts.  It seems like the main challenges there are
related to introspection of changes and mechanisms for combining
changes.

Combining changes is inherently file-format specific, and I suspect
that native authoring tools are best used in those scenarios.
Maybe LFS can help deal with binary conflicts by having short and sweet
ways to grab the "base", "their" and "our" versions of the conflict
files.

Example:

git lfs checkout --theirs --to theirs.wav conflict.wav
git lfs checkout --ours --to ours.wav conflict.wav
git lfs checkout --base --to base.wav conflict.wav

Then the user can use {ours,theirs,base}.wav to produce the
resolved result using their usual authoring tools.

>From the plumbing perspective, we already have the tools to
do this today, but they're not really user-friendly because
they require the user to use "git cat-file --filters --path=..."
and redirect the output to get at their changes.

Not sure if git-lfs is the right place for that kind of helper
wrapper command, but it's not a bad place for it either.
That said, none of these are user-friendly for non-Gits that
might be intimidated by a command-line.

Is there anything we could add to git-cola to help?

Being able to save the different conflicted index stages to
separately named files seems like an obvious feature that
would help users when confronted with a binary conflict.

With LFS and the ongoing work related to MVFS, shallow clone,
and partial checkout, the reasons to use P4 over Git are becoming
less and less compelling.  It'd be great to polish the game asset
workflows further so that we can have a cohesive approach to
doing game asset development using Git that is easy enough for
non-technical users to use and understand.

I mention git-cola because it's a Git porcelain that already has
git-lfs support and I'm very much in favor of improving workflows
related to interacting with LFS, large files, repos, and binary content.

Are there other rough edges around (large) binary files that can be improved?

One thought that comes to mind is diffing -- I imagine that we
might want to use different diff tools depending on the file format.
Currently git-difftool uses a single tool for all files, but it seems
like being able to use different tools, based on the file type, could
be helpful.  Not sure if difftool is the right place for that, but
being able to specify different tools per-file seems be useful in
that scenario.

Another avenue that could use help is documentation about suggested
workflows.  Git's core documentation talks about various
large-file-centric features in isolation, but it'd be good to have a
single user-centric document (not unlike gitworkflows) to document best
practices for dealing with large files, repos, game assets, etc.

That alone would help dispel the myth that Git is unsuitable for
large repos, large files, and binary content.
-- 
David

Re: Feature request: be able to pass arguments to difftool command

[David Aguilar]

On Wed, Aug 29, 2018 at 09:18:38AM +0200, H.Merijn Brand wrote:
> On Tue, 28 Aug 2018 12:37:40 -0700, Junio C Hamano 
> wrote:
> 
> > "H.Merijn Brand"  writes:
> > 
> > > So, my wish would be to have an option, possibly using -- to pass
> > > additional command line arguments to git difftool, so that
> > >
> > >  $ git difftool $commit~1..$commit -- -m -v2
> > >
> > > would pass the arguments after -- transparantly to ccdiff (in my case)  
> > 
> > At the syntax level passing any option after "--" would be a no
> > starter, as I would imagine that "git difftool $revs -- $paths"
> > should still be supported.
> > 
> > At the concept level, however, I can see why such a feature would be
> > useful.  Perhaps
> > 
> > $ git difftool --backend-option=-m --backend-option=-v2 HEAD
> > $ git mergetool --backend-option=--foo
> 
> This would mean I can just pass remaining arguments, like this?
> 
> --8<--- ~/bin/git-ccdiff
> #!/usr/bin/env perl
> use 5.18.3;
> use warnings;
> 
> my $commit;
> 
> @ARGV && $ARGV[0] !~ m/^-/ and $commit = shift;
> 
> my @git = qw( git difftool );
> defined $commit and push @git, "$commit~1..$commit";
> system @git, @ARGV;
> -->8---
> 
> > with appropriate way(s) [*1*] to make it easier to type (and
> > implement) would be an acceptable avenue to pursue, I wonder?
> 
> I like it, as long as they are all separate options in the backend and
> not available in one single variable that needs to be split
> 
> I can envision a configure variable like
> 
>   backends.options.separator = U+2063
> 
> so the backend can safely split on that itself. But I also see this as
> overly complex en over-engineering


Personally, I think it'd be better to keep the tool simple.

While I do see the utility, it would be just as easy to configure a 2nd
and 3rd variant of the same difftool and use those as needed instead.

"git difftool -t ccdiff2" or "-t ccdiff3" is the simplest, and there's
nothing stopping the user from creating aliases to shorten it further.

We also already have, "git difftool -x / --extcmd"
for specifying a full-on external diff command.

>   git -c difftool.ccdiff.opts=-v2 -c difftool.ccdiff.opts=-m difftool

For example, this seems simpler as:

git difftool -x 'ccdiff -v2 -m'

We already have two mechanisms for controlling the inner command that's
launched by difftool.  IMO we don't need more.

My primary concerns with --backend-opts are as follows:

1. If we add a mechansim for passing -X/--backend-opts, then we
   need to specify a new variable that users will need to be aware
   of when creating custom commands.  (sorry for stating the obvious)

2. All of the built-in commands would need to change to honor that
   variable.

3. The documentation becomes more complex because someone that wants
   to configure a bog-standard custom external tool now needs to
   be aware of this extra external source of arguments.

#1 and #2 are primarily implementation concerns, but #3 suggests
to me that it's over-complicating things.

Furthermore, #2 is not really that simple.
What would the sciplet look like?

diff_cmd () {
"$merge_tool_path" $EXTRA_ARGS ...
}

That implies that we would need to shell quote stuff when
constructing $EXTRA_ARGS internally if we were to support multiple -X
arguments.  That just made it a bit more complex.

IMO we should be working to simpliify, not make things more complex for
rare use cases.  There's no reason the user can't just do:

V=2 git difftool
V=3 git difftool

... and let the inner script check for $V (or any other) variable.
While environment variables aren't great, this does seem like the right
place to use them.

Another option -- we already eval the configured command, so if the user
includes a variable ($ARGS) in their custom configuration then they can
specify extra flags today without needing to change the tool.  ex:

[difftool "ccdiff"]
cmd = ccdiff $ARGS \"$LOCAL\" \"$REMOTE\"

ARGS='-v2 -m' git difftool HEAD~1..HEAD


Are these alternatives short and simple enough?
-- 
David

Re: [PATCH] mergetools: add support for guiffy

[David Aguilar]

On Thu, Apr 05, 2018 at 08:55:01AM -0500, Bill Ritcher wrote:
> Add guiffy as difftool and mergetool
> 
> guiffy is available on Windows, Linux, and MacOS
> 
> Signed-off-by: Bill Ritcher <bill_ritc...@guiffy.com>
> ---
>  mergetools/guiffy | 18 ++
>  1 file changed, 18 insertions(+)
>  create mode 100644 mergetools/guiffy
> 
> diff --git a/mergetools/guiffy b/mergetools/guiffy
> new file mode 100644
> index 0..8b23a13c4
> --- /dev/null
> +++ b/mergetools/guiffy
> @@ -0,0 +1,18 @@
> +diff_cmd () {
> + "$merge_tool_path" "$LOCAL" "$REMOTE"
> +}
> +
> +merge_cmd () {
> + if $base_present
> + then
> + "$merge_tool_path" -s "$LOCAL" \
> + "$REMOTE" "$BASE" "$MERGED"
> + else
> + "$merge_tool_path" -m "$LOCAL" \
> +     "$REMOTE" "$MERGED"
> + fi
> +}
> +
> +exit_code_trustable () {
> + true
> +}
> -- 
> 2.15.1.windows.2

I tested this on Linux and it works great.  Thanks Bill.

Acked-by: David Aguilar <dav...@gmail.com>

cheers,
-- 
David

[PATCH v3] mergetools/meld: improve compatibiilty with Meld on macOS X

[David Aguilar]

The macOS X fork of Meld[1] requires a "=" in the "--output"
argument, as it uses a wrapper[2] script that munges the
"--output" argument before calling into the common "meld"
script.

The macOS X wrapper script[2] accepts "--output="
only, despite the fact that the underlying meld code accepts
both "--output <filename" and "--output="[3].

All versions of meld which accept "--output" accept it in
the "--output=" form, so use "--output=" for
maximum compatibility.

[1] https://github.com/yousseb/meld
[2] https://github.com/yousseb/meld/blob/master/osx/Meld
[3] https://github.com/yousseb/meld/issues/42

Reported-by: Matthew Groth <mgrot...@gmail.com>
Helped-by: Samuel Lijin <sxli...@gmail.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
I cloned the meld repo and could not find the code reported in the original
issue, but I did find that same exact code existed in a macOS fork.

After more investigation, this turned out to be a macOS-only issue.  The
commit message has been updated to better reflect what's really going on.

 mergetools/meld | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mergetools/meld b/mergetools/meld
index bc178e8882..7a08470f88 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -10,7 +10,7 @@ merge_cmd () {
 
if test "$meld_has_output_option" = true
then
-   "$merge_tool_path" --output "$MERGED" \
+   "$merge_tool_path" --output="$MERGED" \
"$LOCAL" "$BASE" "$REMOTE"
else
"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
-- 
2.13.1.453.gc0395165f3

[PATCH v2] mergetools/meld: improve backwards-compatibiilty when using "--output"

[David Aguilar]

Meld 3.16.0 requires a "=" in the --output argument, as it uses
a simple hand-rolled command-line parser.

Newer versions of Meld (3.16.4, and possibly earlier) use
optparse, which accepts either "--output " or
"--output=".

Use "--output=" for better compatibility.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
Changes since v1:

Fixed an "optpaarse" -> "optparse" typo in the commit message.
This patch is otherwise identical to v1.

 mergetools/meld | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mergetools/meld b/mergetools/meld
index bc178e8882..7a08470f88 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -10,7 +10,7 @@ merge_cmd () {
 
if test "$meld_has_output_option" = true
then
-   "$merge_tool_path" --output "$MERGED" \
+   "$merge_tool_path" --output="$MERGED" \
"$LOCAL" "$BASE" "$REMOTE"
else
"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
-- 
2.13.1.453.gc0395165f3

Re: [PATCH] mergetools/meld: improve backwards-compatibiilty when using "--output"

[David Aguilar]

On Sun, Jun 18, 2017 at 05:11:48AM -0400, Samuel Lijin wrote:
> On Sun, Jun 18, 2017 at 3:46 AM, David Aguilar <dav...@gmail.com> wrote:
> > On Sat, Jun 17, 2017 at 10:11:36AM -0400, Samuel Lijin wrote:
> >> On Sat, Jun 17, 2017 at 6:24 AM, David Aguilar <dav...@gmail.com> wrote:
> >> > Meld 3.16.0 requires a "=" in the --output argument, as it uses
> >> > a simple hand-rolled command-line parser.
> >> >
> >> > Newer versions of Meld (3.16.4, and possibly earlier) use
> >> > optpaarse, which accepts either "--output " or
> >> > "--output=".
> >
> > Junio, there's an optpaarse -> optparse typo in the commit message
> > here in case you want to fix that up.
> >
> >>
> >> Do older versions also support both?
> >
> > No.  When the "--output" option was first added (3.16.0, or possibly
> > earlier) it used the simpler parser that does not undertand the
> > "--output " form.
> >
> > Much older versions didn't support "--output" at all, so we don't have
> > to worry about them since we already use the "--output" flag
> > selectively based on whether or not it's supported.
> 
> It sounds like this patch would break versions of Meld that use the
> hand-rolled parser, then.

I don't think so.

The whole point of this patch is to make it compatible with the
hand-rolled parser.

Before the patch:

--output 

After the patch:

--output=


The form with "=" (the latter one) is the one that's maximally
compatible.

Please re-read the commit message and patch to verify that this is
indeed true.
-- 
David

Re: [PATCH] mergetools/meld: improve backwards-compatibiilty when using "--output"

[David Aguilar]

On Sat, Jun 17, 2017 at 10:11:36AM -0400, Samuel Lijin wrote:
> On Sat, Jun 17, 2017 at 6:24 AM, David Aguilar <dav...@gmail.com> wrote:
> > Meld 3.16.0 requires a "=" in the --output argument, as it uses
> > a simple hand-rolled command-line parser.
> >
> > Newer versions of Meld (3.16.4, and possibly earlier) use
> > optpaarse, which accepts either "--output " or
> > "--output=".

Junio, there's an optpaarse -> optparse typo in the commit message
here in case you want to fix that up.

> 
> Do older versions also support both?

No.  When the "--output" option was first added (3.16.0, or possibly
earlier) it used the simpler parser that does not undertand the
"--output " form.

Much older versions didn't support "--output" at all, so we don't have
to worry about them since we already use the "--output" flag
selectively based on whether or not it's supported.
-- 
David

[PATCH] mergetools/meld: improve backwards-compatibiilty when using "--output"

[David Aguilar]

Meld 3.16.0 requires a "=" in the --output argument, as it uses
a simple hand-rolled command-line parser.

Newer versions of Meld (3.16.4, and possibly earlier) use
optpaarse, which accepts either "--output " or
"--output=".

Use "--output=" for better compatibility.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 mergetools/meld | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mergetools/meld b/mergetools/meld
index bc178e8882..7a08470f88 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -10,7 +10,7 @@ merge_cmd () {
 
if test "$meld_has_output_option" = true
then
-   "$merge_tool_path" --output "$MERGED" \
+   "$merge_tool_path" --output="$MERGED" \
"$LOCAL" "$BASE" "$REMOTE"
else
"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
-- 
2.13.1.453.gc0395165f3

Re: [PATCH] contrib/subtree: add "--no-commit" flag for merge and pull

[David Aguilar]

On Sun, Mar 26, 2017 at 03:02:38AM -0400, Mike Lewis wrote:
> Allows the user to verify and/or change the contents of the merge
> before committing as necessary
> 
> Signed-off-by: Mike Lewis 
> ---
>  contrib/subtree/git-subtree.sh | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index dec085a23..c30087485 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -29,6 +29,8 @@ onto= try connecting new tree to an existing one
>  rejoinmerge the new branch back into HEAD
>   options for 'add', 'merge', and 'pull'
>  squashmerge subtree changes as a single commit
> + options for 'merge' and 'pull'
> +no-commit perform the merge, but don't commit
>  "
>  eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit 
> $?)"
>  
> @@ -48,6 +50,7 @@ annotate=
>  squash=
>  message=
>  prefix=
> +commit_option="--commit"

It might be simpler to default commit_option= empty like the others, and
remove the "" double quotes around "$commit_option" indicated below so
that the shell ignores it when it's empty.

>  
>  debug () {
>   if test -n "$debug"
> @@ -137,6 +140,12 @@ do
>   --no-squash)
>   squash=
>   ;;
> + --no-commit)
> + commit_option="--no-commit"
> + ;;
> + --no-no-commit)
> + commit_option="--commit"
> + ;;

"--no-no-commit" should just be "--commit" instead.
The real flag is called "--commit" (git help merge), so subtree
should follow suite by supporting "--commit" and "--no-commit" only.


> @@ -815,17 +824,17 @@ cmd_merge () {
>   then
>   if test -n "$message"
>   then
> - git merge -s subtree --message="$message" "$rev"
> + git merge -s subtree --message="$message" 
> "$commit_option" "$rev"
  ^ 
 ^
>   else
> - git merge -s subtree "$rev"
> + git merge -s subtree "$commit_option" "$rev"
 ^  ^
>   fi
>  [...]

-- 
David

Re: should git-subtree ignore submodules?

[David Aguilar]

On Tue, Nov 01, 2016 at 12:41:50PM -0700, Kirill Katsnelson wrote:
> "git-subtree add" fails to add the subtree into a repository with
> submodules, reporting that the worktree is not clean, while `git status`
> says that everything is clean (including submodules). I tracked the problem
> down to the command "git index HEAD" returning all submodules as having the
> M status:
> 
> $ git diff-index HEAD
> :16 16 d3812c9318c4d0336897fd2d666be908fa1a7953
> d3812c9318c4d0336897fd2d666be908fa1a7953 M  ext/grpc
> 
> $ git --version
> git version 2.9.2.windows.1
> 
> I worked around the problem in my local copy of git-subtree shell script by
> adding "--ignore-submodules=all" to the two invocations of `git diff-index`
> in the ensure_clean() function (direct link
> ).
> 
> I am wondering, is this a defect in git-subtree? To my understanding, the
> command should not care about submodules more than ensuring their worktree
> is not in the way of new prefix, and that's a separate check. So *even if*
> the submodule is modified, this should not be a show stopper for
> "git-subtree add". Or am I missing some subtleties?

That sounds correct to me; subtree should ignore submodules.
-- 
David

Re: [PATCH v2 3/3] difftool: handle modified symlinks in dir-diff mode

[David Aguilar]

On Wed, Mar 15, 2017 at 11:54:14AM -0700, Junio C Hamano wrote:
> David Aguilar <dav...@gmail.com> writes:
> 
> > @@ -397,7 +438,7 @@ static int run_dir_diff(const char *extcmd, int 
> > symlinks, const char *prefix,
> > return error("could not write '%s'", src_path);
> > }
> >  
> > -   if (rmode) {
> > +   if (rmode && !S_ISLNK(rmode)) {
> > struct working_tree_entry *entry;
> 
> It still makes me wonder why the new !S_ISLNK() is done only for the
> right hand side, though.  In fact, the processing done for the left
> hand side and the right hand side are vastly different even without
> this patch.
> 
> I suspect this is probably because the code is not prepared to drive
> the underlying "diff" when given the "-R" option (if I am reading
> the code correctly, argv[] that came from the end-user is appended
> to "diff --raw --no-abbrev -z", so the user could ask "difftool
> --dir-diff -R ..."), in which case you would see the working tree
> files as the left hand side of the diff.  In the dir-diff mode,
> because you want to make only the working-tree side writable (and
> reflect whatever edit the user made back to the working-tree side),
> the choices you have to fix it would either be forbid "-R" (which is
> less preferrable as it is a more brittle solution between the two)
> or read the "diff --raw" output and swap the sides when you notice
> that LHS has 0{40} with non 0 mode, which is a sign that that side
> represents the working tree.
> 
> Having said all that, let's focus on the "symlink" stuff in this
> series.
> 
> Thanks.

Agreed.  I can take a look at the reverse-diff
question later this week separately from this issue.

The symlink test case I just added can be used as a starting
ground for adding reverse-diff tests.

Thanks Junio and Dscho for the reviews,
-- 
David

[PATCH v2 3/3] difftool: handle modified symlinks in dir-diff mode

[David Aguilar]

Detect the null object ID for symlinks in dir-diff so that difftool can
detect when symlinks are modified in the worktree.

Previously, a null symlink object ID would crash difftool.
Handle null object IDs as unknown content that must be read from
the worktree.

Helped-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
Only 3/3 was re-sent; the rest are the same.

When re-reading the patch I noticed two spots where spurious
whitespace was added.  I've dropped those hunks.

 builtin/difftool.c  | 51 -
 t/t7800-difftool.sh | 60 +
 2 files changed, 106 insertions(+), 5 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index d13350ce83..25e54ad3ed 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -254,6 +254,49 @@ static int ensure_leading_directories(char *path)
}
 }
 
+/*
+ * Unconditional writing of a plain regular file is what
+ * "git difftool --dir-diff" wants to do for symlinks.  We are preparing two
+ * temporary directories to be fed to a Git-unaware tool that knows how to
+ * show a diff of two directories (e.g. "diff -r A B").
+ *
+ * Because the tool is Git-unaware, if a symbolic link appears in either of
+ * these temporary directories, it will try to dereference and show the
+ * difference of the target of the symbolic link, which is not what we want,
+ * as the goal of the dir-diff mode is to produce an output that is logically
+ * equivalent to what "git diff" produces.
+ *
+ * Most importantly, we want to get textual comparison of the result of the
+ * readlink(2).  get_symlink() provides that---it returns the contents of
+ * the symlink that gets written to a regular file to force the external tool
+ * to compare the readlink(2) result as text, even on a filesystem that is
+ * capable of doing a symbolic link.
+ */
+static char *get_symlink(const struct object_id *oid, const char *path)
+{
+   char *data;
+   if (is_null_oid(oid)) {
+   /* The symlink is unknown to Git so read from the filesystem */
+   struct strbuf link = STRBUF_INIT;
+   if (has_symlinks) {
+   if (strbuf_readlink(, path, strlen(path)))
+   die(_("could not read symlink %s"), path);
+   } else if (strbuf_read_file(, path, 128))
+   die(_("could not read symlink file %s"), path);
+
+   data = strbuf_detach(, NULL);
+   } else {
+   enum object_type type;
+   unsigned long size;
+   data = read_sha1_file(oid->hash, , );
+   if (!data)
+   die(_("could not read object %s for symlink %s"),
+   oid_to_hex(oid), path);
+   }
+
+   return data;
+}
+
 static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
int argc, const char **argv)
 {
@@ -270,8 +313,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
struct hashmap working_tree_dups, submodules, symlinks2;
struct hashmap_iter iter;
struct pair_entry *entry;
-   enum object_type type;
-   unsigned long size;
struct index_state wtindex;
struct checkout lstate, rstate;
int rc, flags = RUN_GIT_CMD, err = 0;
@@ -377,13 +418,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
}
 
if (S_ISLNK(lmode)) {
-   char *content = read_sha1_file(loid.hash, , );
+   char *content = get_symlink(, src_path);
add_left_or_right(, src_path, content, 0);
free(content);
}
 
if (S_ISLNK(rmode)) {
-   char *content = read_sha1_file(roid.hash, , );
+   char *content = get_symlink(, dst_path);
add_left_or_right(, dst_path, content, 1);
free(content);
}
@@ -397,7 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
return error("could not write '%s'", src_path);
}
 
-   if (rmode) {
+   if (rmode && !S_ISLNK(rmode)) {
struct working_tree_entry *entry;
 
/* Avoid duplicate working_tree entries */
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e0e65df8de..0e7f30db2d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -626,4 +626,64 @@ test_expect_success SYMLINKS 'difftool --dir-diff 
symlinked directories' '
)
 '
 
+test_expect_success SYMLINKS 'difftool --dir-diff handles modified sy

[PATCH 2/3] t7800: cleanup cruft left behind by tests

[David Aguilar]

Signed-off-by: David Aguilar <dav...@gmail.com>
---
More cleanup, this is needed because the final patch adds a test
to t7800 that was sensitive to the cruft left behind.

 t/t7800-difftool.sh | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e1ec292718..e0e65df8de 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -591,6 +591,7 @@ test_expect_success 'difftool --no-symlinks detects 
conflict ' '
 '
 
 test_expect_success 'difftool properly honors gitlink and core.worktree' '
+   test_when_finished rm -rf submod/ule &&
git submodule add ./. submod/ule &&
test_config -C submod/ule diff.tool checktrees &&
test_config -C submod/ule difftool.checktrees.cmd '\''
@@ -600,11 +601,13 @@ test_expect_success 'difftool properly honors gitlink and 
core.worktree' '
cd submod/ule &&
echo good >expect &&
git difftool --tool=checktrees --dir-diff HEAD~ >actual &&
-   test_cmp expect actual
+   test_cmp expect actual &&
+   rm -f expect actual
)
 '
 
 test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '
+   test_when_finished git reset --hard &&
git init dirlinks &&
(
cd dirlinks &&
-- 
2.12.0.309.gffef9e61c2

[PATCH 3/3] difftool: handle modified symlinks in dir-diff mode

[David Aguilar]

Detect the null object ID for symlinks in dir-diff so that difftool can
detect when symlinks are modified in the worktree.

Previously, a null symlink object ID would crash difftool.
Handle null object IDs as unknown content that must be read from
the worktree.

Helped-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
This was reworked a bit since the original patch.
The subject line changed, a lot of comments were added,
and the tests were made more extensive.

This implementation is simpler since we now use a get_symlink()
function in one place and simply skip the problematic code path
that does not apply to symlinks.  Existing code handles writing
the symlink content so write_symlink_file() from the original
patch is gone.

 builtin/difftool.c  | 53 +-
 t/t7800-difftool.sh | 60 +
 2 files changed, 108 insertions(+), 5 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index d13350ce83..6aab5cd23a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -254,6 +254,49 @@ static int ensure_leading_directories(char *path)
}
 }
 
+/*
+ * Unconditional writing of a plain regular file is what
+ * "git difftool --dir-diff" wants to do for symlinks.  We are preparing two
+ * temporary directories to be fed to a Git-unaware tool that knows how to
+ * show a diff of two directories (e.g. "diff -r A B").
+ *
+ * Because the tool is Git-unaware, if a symbolic link appears in either of
+ * these temporary directories, it will try to dereference and show the
+ * difference of the target of the symbolic link, which is not what we want,
+ * as the goal of the dir-diff mode is to produce an output that is logically
+ * equivalent to what "git diff" produces.
+ *
+ * Most importantly, we want to get textual comparison of the result of the
+ * readlink(2).  get_symlink() provides that---it returns the contents of
+ * the symlink that gets written to a regular file to force the external tool
+ * to compare the readlink(2) result as text, even on a filesystem that is
+ * capable of doing a symbolic link.
+ */
+static char *get_symlink(const struct object_id *oid, const char *path)
+{
+   char *data;
+   if (is_null_oid(oid)) {
+   /* The symlink is unknown to Git so read from the filesystem */
+   struct strbuf link = STRBUF_INIT;
+   if (has_symlinks) {
+   if (strbuf_readlink(, path, strlen(path)))
+   die(_("could not read symlink %s"), path);
+   } else if (strbuf_read_file(, path, 128))
+   die(_("could not read symlink file %s"), path);
+
+   data = strbuf_detach(, NULL);
+   } else {
+   enum object_type type;
+   unsigned long size;
+   data = read_sha1_file(oid->hash, , );
+   if (!data)
+   die(_("could not read object %s for symlink %s"),
+   oid_to_hex(oid), path);
+   }
+
+   return data;
+}
+
 static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
int argc, const char **argv)
 {
@@ -270,8 +313,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
struct hashmap working_tree_dups, submodules, symlinks2;
struct hashmap_iter iter;
struct pair_entry *entry;
-   enum object_type type;
-   unsigned long size;
struct index_state wtindex;
struct checkout lstate, rstate;
int rc, flags = RUN_GIT_CMD, err = 0;
@@ -377,13 +418,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
}
 
if (S_ISLNK(lmode)) {
-   char *content = read_sha1_file(loid.hash, , );
+   char *content = get_symlink(, src_path);
add_left_or_right(, src_path, content, 0);
free(content);
}
 
if (S_ISLNK(rmode)) {
-   char *content = read_sha1_file(roid.hash, , );
+   char *content = get_symlink(, dst_path);
add_left_or_right(, dst_path, content, 1);
free(content);
}
@@ -397,7 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
return error("could not write '%s'", src_path);
}
 
-   if (rmode) {
+   if (rmode && !S_ISLNK(rmode)) {
struct working_tree_entry *entry;
 
/* Avoid duplicate working_tree entries */
@@ -414,6 +455,7 @@ static int run_dir_diff(const

[PATCH 1/3] t7800: remove whitespace before redirect

[David Aguilar]

Signed-off-by: David Aguilar <dav...@gmail.com>
---
Cleanup before the fix.

 t/t7800-difftool.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 25241f4096..e1ec292718 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -428,7 +428,7 @@ run_dir_diff_test 'difftool --dir-diff branch from 
subdirectory' '
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
# "sub" must only exist in "right"
# "file" and "file2" must be listed in both "left" and "right"
-   grep sub output > sub-output &&
+   grep sub output >sub-output &&
test_line_count = 1 sub-output &&
grep file"$" output >file-output &&
test_line_count = 2 file-output &&
-- 
2.12.0.309.gffef9e61c2

Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[David Aguilar]

On Mon, Mar 13, 2017 at 02:33:09PM -0700, Junio C Hamano wrote:
> Johannes Schindelin  writes:
> 
> >> > +if (strbuf_readlink(, ce->name, ce_namelen(ce)) == 0) {
> >> > +strbuf_add(, state->base_dir, state->base_dir_len);
> >> > +strbuf_add(, ce->name, ce_namelen(ce));
> >> > +
> >> > +write_file_buf(path.buf, link.buf, link.len);
> >> 
> >> This does "write content into symlink stand-in file", but why?
> >
> > From the commit message:
> >
> > > Detect the null object ID for symlinks in dir-diff so that
> > > difftool can prepare temporary files that matches how git
> > > handles symlinks.
> 
> Yes I read what the proposed log message said, and noticed that
> symbolic link is _always_ written as a regular file.
> 
> I was questioning why.  I know Git falls back to such behaviour when
> the filesystem does not support symbolic links.  "Why do so
> unconditionally, even when the filesystem does?" was the question.
> 
> > The obvious connection: when core.symlinks = false, Git already falls back
> > to writing plain files with the link target as contents. This function
> > does the same, for the same motivation: it is the best we can do in this
> > case.
> 
> And that "obvious connection" does not answer the question.

Thanks for the thorough review.

I'll try to answer questions from the various emails in just this
one spot in case it helps.

Dscho wrote:
> Given that we explicitly ask use_wt_file() whether we can use the
> worktree's file, and we get the answer "no" before we enter the modified
> code block, I would really expect us *not* to want to read the link from
> disk at all.

That probably deserves a comment on its own.

The use_wt_file() function really answers the question --
"does the worktree contain content that does is unknown to
 Git, and thus we should symlink to the worktree?"

Since these are symlinks, and symlinks are already used to
point back to the worktree (so that difftools will write
back to the worktree in case the user edited in-tool)
then the meaning of use_wt_file() in this context
can be misleading.

What we're trying to do is handle the case where Git knows it's dealing
with an entry that it wants to checkout into a temporary area
but it has no way to do so.  These entries always have the
0{40} null SHA1 because that is what git diff-files reports
for content that exists in the worktree only.

Dscho wrote:
> I think you are right, we cannot simply call strbuf_readlink(), we would
> have to check the core_symlinks variable to maybe read the file contents
> instead.
>
> But then, it may not be appropriate to read the worktree to begin
> with...
> see my reply to the patch that I will send out in a couple of minutes.

In this case, where the null SHA1 indicates that it is unknown content,
then I believe we must read from the worktree to simulate what Git
would have checked out.  Checking for core.symlinks should probably be
done before calling strbuf_readlink, though.

Junio wrote:
> > > Detect the null object ID for symlinks in dir-diff so that
> > > difftool can prepare temporary files that matches how git
> > > handles symlinks.
>
> Yes I read what the proposed log message said, and noticed that
> symbolic link is _always_ written as a regular file.
>
> I was questioning why.  I know Git falls back to such behaviour when
> the filesystem does not support symbolic links.  "Why do so
> unconditionally, even when the filesystem does?" was the question.

I have to re-read the code to see where this is special-cased, but
it seems that symlinks are always written as raw files by the
dir-diff code for the purposes of full-tree diffing.

I think the "why" is tied up in the implementation details of
the symlink-back-to-the-worktree-to-allow-editing feature.
By special-casing in-tree symlinks and writing them as raw
files we can hijack on-disk symlinks.  We use on-disk symlinks to
link back to the worktree so that external diff tools can write
to the worktree through the symlink.

Junio wrote:
> On this part I didn't comment in my previous message, but what is
> the implication of omitting add-left-or-right and not registering
> this symbolic link modified in the working tree to the symlinks2
> table?
>
> I am wondering if these should be more like
>
> if (S_ISLNK(lmode) {
> char *content = get_symlink(src_path, );
> add_left_or_right(, src_path, content, 0);
> free(content);
> }
>
> with get_symlink() helper that does
>
>  - if the object name is not 0{40}, read from the object store
>
>  - if the object name is 0{40}, that means we need to read the real
>contents from the working tree file, so do the "readlink(2) if
>symbolic link is supported, otherwise open/read/close the stub
>file sitting there" thing.
>
> Similary to the right hand side tree.

I'll take a look at trying this out.

Reading the code again, the 

[PATCH] difftool: handle changing symlinks in dir-diff mode

[David Aguilar]

Detect the null object ID for symlinks in dir-diff so that difftool can
prepare temporary files that matches how git handles symlinks.

Previously, a null object ID would crash difftool.  We now detect null
object IDs and write the symlink's content into the temporary symlink
stand-in file.

Original-patch-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
 builtin/difftool.c  | 36 +---
 t/t7800-difftool.sh | 40 
 2 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index d13350ce83..6c20e20b45 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -254,6 +254,31 @@ static int ensure_leading_directories(char *path)
}
 }
 
+static int create_symlink_file(struct cache_entry* ce, struct checkout* state)
+{
+   /*
+* Dereference a worktree symlink and writes its contents
+* into the checkout state's path.
+*/
+   struct strbuf path = STRBUF_INIT;
+   struct strbuf link = STRBUF_INIT;
+
+   int ok = 0;
+
+   if (strbuf_readlink(, ce->name, ce_namelen(ce)) == 0) {
+   strbuf_add(, state->base_dir, state->base_dir_len);
+   strbuf_add(, ce->name, ce_namelen(ce));
+
+   write_file_buf(path.buf, link.buf, link.len);
+   ok = 1;
+   }
+
+   strbuf_release();
+   strbuf_release();
+
+   return ok;
+}
+
 static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
int argc, const char **argv)
 {
@@ -376,13 +401,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
continue;
}
 
-   if (S_ISLNK(lmode)) {
+   if (S_ISLNK(lmode) && !is_null_oid()) {
char *content = read_sha1_file(loid.hash, , );
add_left_or_right(, src_path, content, 0);
free(content);
}
 
-   if (S_ISLNK(rmode)) {
+   if (S_ISLNK(rmode) && !is_null_oid()) {
char *content = read_sha1_file(roid.hash, , );
add_left_or_right(, dst_path, content, 1);
free(content);
@@ -414,7 +439,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
oidcpy(>oid, );
strcpy(ce->name, dst_path);
ce->ce_namelen = dst_path_len;
-   if (checkout_entry(ce, , NULL))
+
+   if (S_ISLNK(rmode) && is_null_oid()) {
+   if (!create_symlink_file(ce, ))
+   return error("unable to create 
symlink file %s",
+dst_path);
+   } else if (checkout_entry(ce, , NULL))
return error("could not write '%s'",
 dst_path);
} else if (!is_null_oid()) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e1ec292718..64f8e451b5 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -623,4 +623,44 @@ test_expect_success SYMLINKS 'difftool --dir-diff 
symlinked directories' '
)
 '
 
+test_expect_success SYMLINKS 'difftool --dir-diff' '
+   touch b &&
+   ln -s b c &&
+   git add . &&
+   test_tick &&
+   git commit -m initial &&
+   touch d &&
+   rm c &&
+   ln -s d c &&
+
+   git difftool --dir-diff --extcmd ls >output &&
+   grep -v ^/ output >actual &&
+   cat >expect <<-EOF &&
+   b
+   c
+   dirlinks
+   output
+   submod
+
+   c
+   dirlinks
+   output
+   submod
+   EOF
+   test_cmp expect actual &&
+
+   # The left side contains symlink "c" that points to "b"
+   test_config difftool.cat.cmd "cat \$LOCAL/c" &&
+   git difftool --dir-diff --tool cat >actual &&
+   echo b >expect &&
+   test_cmp expect actual &&
+
+   # The right side contains symlink "c" that points to "d",
+   # which mimics the state of the worktree.
+   test_config difftool.cat.cmd "cat \$REMOTE/c" &&
+   git difftool --dir-diff --tool cat >actual &&
+   echo -n d >expect &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.12.0.266.g44c9eec009

Re: [PATCH 3/3] Remove outdated info in difftool manpage

[David Aguilar]

On Fri, Mar 03, 2017 at 01:28:36PM -0800, Denton Liu wrote:
> On Fri, Mar 03, 2017 at 04:46:36PM +0100, Johannes Schindelin wrote:
> > Hi Denton (or should I address you as Liu?),
> Denton is fine, thanks.
> > 
> > On Fri, 3 Mar 2017, Denton Liu wrote:
> > 
> > > When difftool was rewritten in C, it removed the capability to read
> > > fallback configs from mergetool. This changes the documentation to
> > > reflect this.
> > 
> > Thanks for pointing that out. But that is probably an oversight on my
> > part, not an intentional change...
> Do you expect to be submitting a patch for this soon? Or, if not, would
> it be fine if I went ahead and did it?

Thanks for spotting this.  It'd be good to fix this so
I'm sure no one would mind if you submitted a patch ;-)

I'd be happy to test your patch if you have one.
-- 
David

Re: [PATCH 1/3] Add --gui option to mergetool

[David Aguilar]

On Fri, Mar 03, 2017 at 03:57:38AM -0800, Denton Liu wrote:
> This fixes the discrepancy between difftool and mergetool where the
> former has the --gui flag and the latter does not by adding the
> functionality to mergetool.
> 
> Signed-off-by: Denton Liu 
> ---
>  Documentation/git-mergetool.txt|  8 +++-
>  contrib/completion/git-completion.bash |  3 ++-
>  git-mergetool.sh   |  5 -
>  t/t7610-mergetool.sh   | 28 +++-
>  4 files changed, 40 insertions(+), 4 deletions(-)

Would you mind splitting up this patch so that the
completion part is done separately?


> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index 381b7df45..5683907ab 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -145,6 +147,30 @@ test_expect_success 'custom mergetool' '
>   git commit -m "branch1 resolved with mergetool"
>  '
>  
> +test_expect_success 'gui mergetool' '
> + test_when_finished "git reset --hard" &&
> + test_when_finished "git config merge.tool mytool" &&
> + test_when_finished "git config --unset merge.guitool" &&
> + git config merge.tool badtool &&
> + git config merge.guitool mytool &&
> + git checkout -b test$test_count branch1 &&
> + git submodule update -N &&
> + test_must_fail git merge master >/dev/null 2>&1 &&

It'd probably be better to use test_expect_code instead of
test_must_fail here:

test_expect_code 1 git merge master ...


> + ( yes "" | git mergetool -g both >/dev/null 2>&1 ) &&
> + ( yes "" | git mergetool -g file1 file1 ) &&
> + ( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
> + ( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&

I realize that this test is based on an existing one, but I'm curious..
is "yes" used above because it's prompting, and using -y or --no-prompt
here would eliminate the need for the 'yes ""' parts?

Looks good otherwise.

Thanks,
-- 
David

Re: subtree merging fails

[David Aguilar]

On Tue, Feb 07, 2017 at 08:59:06AM -0600, Samuel Lijin wrote:
> Have you tried using (without -s subtree) -X subtree=path/to/add/subtree/at?
> 
> From the man page:
> 
>   subtree[=]
>This option is a more advanced form of subtree
> strategy, where the strategy
>makes a guess on how two trees must be shifted to match
> with each other when
>merging. Instead, the specified path is prefixed (or
> stripped from the
>beginning) to make the shape of two trees to match.

I'm not 100% certain, but it's highly likely that the subtree=
argument needs to include a trailing slash "/" in the prefix,
otherwise files will be named e.g. "fooREADME" instead of
"foo/README" when prefix=foo.

These days I would steer users towards the "git-subtree" command in
contrib/ so that users don't need to deal with these details.  It
handles all of this stuff for you.

https://github.com/git/git/blob/master/contrib/subtree/git-subtree.txt

https://github.com/git/git/tree/master/contrib/subtree

Updating the progit book to also mention git-subtree, in addition to the
low-level methods, would probably be a good user-centric change.
-- 
David

[PATCH 2/2] t7800: replace "wc -l" with test_line_count

[David Aguilar]

Make t7800 easier to debug by capturing output into temporary files and
using test_line_count to make assertions on those files.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 t/t7800-difftool.sh | 31 +++
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 97bae54d83..25241f4096 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -290,8 +290,8 @@ test_expect_success 'difftool + mergetool config variables' 
'
 test_expect_success 'difftool..path' '
test_config difftool.tkdiff.path echo &&
git difftool --tool=tkdiff --no-prompt branch >output &&
-   lines=$(grep file output | wc -l) &&
-   test "$lines" -eq 1
+   grep file output >grep-output &&
+   test_line_count = 1 grep-output
 '
 
 test_expect_success 'difftool --extcmd=cat' '
@@ -428,9 +428,12 @@ run_dir_diff_test 'difftool --dir-diff branch from 
subdirectory' '
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
# "sub" must only exist in "right"
# "file" and "file2" must be listed in both "left" and "right"
-   test "1" = $(grep sub output | wc -l) &&
-   test "2" = $(grep file"$" output | wc -l) &&
-   test "2" = $(grep file2 output | wc -l)
+   grep sub output > sub-output &&
+   test_line_count = 1 sub-output &&
+   grep file"$" output >file-output &&
+   test_line_count = 2 file-output &&
+   grep file2 output >file2-output &&
+   test_line_count = 2 file2-output
)
 '
 
@@ -440,9 +443,11 @@ run_dir_diff_test 'difftool --dir-diff v1 from 
subdirectory' '
git difftool --dir-diff $symlinks --extcmd ls v1 >output &&
# "sub" and "file" exist in both v1 and HEAD.
# "file2" is unchanged.
-   test "2" = $(grep sub output | wc -l) &&
-   test "2" = $(grep file output | wc -l) &&
-   test "0" = $(grep file2 output | wc -l)
+   grep sub output >sub-output &&
+   test_line_count = 2 sub-output &&
+   grep file output >file-output &&
+   test_line_count = 2 file-output &&
+   ! grep file2 output
)
 '
 
@@ -452,8 +457,9 @@ run_dir_diff_test 'difftool --dir-diff branch from 
subdirectory w/ pathspec' '
git difftool --dir-diff $symlinks --extcmd ls branch -- 
.>output &&
# "sub" only exists in "right"
# "file" and "file2" must not be listed
-   test "1" = $(grep sub output | wc -l) &&
-   test "0" = $(grep file output | wc -l)
+   grep sub output >sub-output &&
+   test_line_count = 1 sub-output &&
+   ! grep file output
)
 '
 
@@ -463,8 +469,9 @@ run_dir_diff_test 'difftool --dir-diff v1 from subdirectory 
w/ pathspec' '
git difftool --dir-diff $symlinks --extcmd ls v1 -- .>output &&
# "sub" exists in v1 and HEAD
# "file" is filtered out by the pathspec
-   test "2" = $(grep sub output | wc -l) &&
-   test "0" = $(grep file output | wc -l)
+   grep sub output >sub-output &&
+   test_line_count = 2 sub-output &&
+   ! grep file output
)
 '
 
-- 
2.12.0.rc0.228.g6c028b8e94

[PATCH 1/2] t7800: simplify basic usage test

[David Aguilar]

Use "test_line_count" instead of "wc -l", use "git -C" instead of a
subshell, and use test_expect_code when calling difftool.  Ease
debugging by capturing output into temporary files.

Suggested-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
This patch applies on top of js/difftool-builtin in next
"difftool: fix bug when printing usage"

 t/t7800-difftool.sh | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 21e2ac4ad6..97bae54d83 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -24,16 +24,15 @@ prompt_given ()
 }
 
 test_expect_success 'basic usage requires no repo' '
-   lines=$(git difftool -h | grep ^usage: | wc -l) &&
-   test "$lines" -eq 1 &&
+   test_expect_code 129 git difftool -h >output &&
+   grep ^usage: output &&
# create a ceiling directory to prevent Git from finding a repo
mkdir -p not/repo &&
-   ceiling="$PWD/not" &&
-   lines=$(cd not/repo &&
-   GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
-   grep ^usage: | wc -l) &&
-   test "$lines" -eq 1 &&
-   rmdir -p not/repo
+   test_when_finished rm -r not &&
+   test_expect_code 129 \
+   env GIT_CEILING_DIRECTORIES="$(pwd)/not" \
+   git -C not/repo difftool -h >output &&
+   grep ^usage: output
 '
 
 # Create a file on master and change it on branch
-- 
2.12.0.rc0.228.g6c028b8e94

[PATCH] difftool: fix bug when printing usage

[David Aguilar]

"git difftool -h" reports an error:

fatal: BUG: setup_git_env called without repository

Defer repository setup so that the help option processing happens before
the repository is initialized.

Add tests to ensure that the basic usage works inside and outside of a
repository.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
This bug exists in both "master" and "next".
This patch has been tested on both branches.

 builtin/difftool.c  |  8 
 t/t7800-difftool.sh | 13 +
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index b5e85ab079..d13350ce83 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -647,10 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   /* NEEDSWORK: once we no longer spawn anything, remove this */
-   setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
-   setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 
1);
-
git_config(difftool_config, NULL);
symlinks = has_symlinks;
 
@@ -661,6 +657,10 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
if (tool_help)
return print_tool_help();
 
+   /* NEEDSWORK: once we no longer spawn anything, remove this */
+   setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
+   setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 
1);
+
if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
else if (difftool_cmd) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index aa0ef02597..21e2ac4ad6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,19 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
 }
 
+test_expect_success 'basic usage requires no repo' '
+   lines=$(git difftool -h | grep ^usage: | wc -l) &&
+   test "$lines" -eq 1 &&
+   # create a ceiling directory to prevent Git from finding a repo
+   mkdir -p not/repo &&
+   ceiling="$PWD/not" &&
+   lines=$(cd not/repo &&
+   GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
+   grep ^usage: | wc -l) &&
+   test "$lines" -eq 1 &&
+   rmdir -p not/repo
+'
+
 # Create a file on master and change it on branch
 test_expect_success 'setup' '
echo master >file &&
-- 
2.12.0.rc0.209.g3d1e53e462

[BUG] was: Re: [PATCH] Remove --no-gui option from difftool usage string

[David Aguilar]

On Fri, Feb 03, 2017 at 06:56:17PM -0800, Denton Liu wrote:
> The --no-gui option not documented in the manpage, nor is it actually
> used in the source code. This change removes it from the usage help
> that's printed.
> 
> Signed-off-by: Denton Liu 
> ---
>  git-difftool.perl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

[Dscho, I found a bug, see below]

I forgot to mention that the scripted difftool has been
rewritten in C and will be going away soon.
builtin/difftool.c is already in "next".

New patches against difftool should target the builtin.

Regarding removing it from the usage string, IMO that can be
considered a good change if the rationale were instead that
we never expect users to ever type "--no-gui" in practice.

Wasting the short usage string screen space with a useless to
99.99% users option is bad for usability.  From that perspective
we shouldn't mention it there, so reframing the commit message
towards that argument would make for a better motivation.

Removing the mention from the usage string and adding it to the
manpage would be the a good change from that perspective as well.


BTW, in "next", it seems that the builtin difftool crashes when
doing "git difftool -h", so we should probably add a test
for that too...

>From the tip of next:

$ git difftool -h
fatal: BUG: setup_git_env called without repository
-- 
David

Re: [PATCH] Remove --no-gui option from difftool usage string

[David Aguilar]

On Fri, Feb 03, 2017 at 10:23:51PM -0800, Denton Liu wrote:
> On Fri, Feb 03, 2017 at 09:58:09PM -0800, Jacob Keller wrote:
> > On Fri, Feb 3, 2017 at 6:56 PM, Denton Liu  wrote:
> > > The --no-gui option not documented in the manpage, nor is it actually
> > > used in the source code. This change removes it from the usage help
> > > that's printed.
> > >
> > > Signed-off-by: Denton Liu 
> > > ---
> > >  git-difftool.perl | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/git-difftool.perl b/git-difftool.perl
> > > index a5790d03a..657d5622d 100755
> > > --- a/git-difftool.perl
> > > +++ b/git-difftool.perl
> > > @@ -29,8 +29,8 @@ sub usage
> > > print << 'USAGE';
> > >  usage: git difftool [-t|--tool=] [--tool-help]
> > >  [-x|--extcmd=]
> > > -[-g|--gui] [--no-gui]
> > > -[--prompt] [-y|--no-prompt]
> > > +[-g|--gui] [--prompt]
> > > +[-y|--no-prompt]
> > >  [-d|--dir-diff]
> > >  ['git diff' options]
> > >  USAGE
> > > --
> > > 2.11.0
> > >
> > 
> > Aren't "--no-foo" options automatically created for booleans when
> > using our option parsing code?
> > 
> > Thanks,
> > Jake
> 
> Sorry, I guess I didn't notice that. Would it make sense to document it
> in the manpage then?

The general "--no-*" convention is mentioned in "git help cli",
but the manpages and usage strings across all commands are
inconsistent about mentioning the "--no-*" variants; some do,
some don't.

IMO it probably wouldn't hurt to document --no-gui in the manpage.

cheers,
-- 
David

Re: [PATCH] Add --gui option to mergetool

[David Aguilar]

On Fri, Feb 03, 2017 at 10:43:03PM -0800, Denton Liu wrote:
> * fix the discrepancy between difftool and mergetool where
>   the former has the --gui flag and the latter does not by adding the
>   functionality to mergetool

Please avoid bullet points in commit messages when a simple
paragraph will suffice.

The implementation of this feature seems ok, but tests are
needed in t/t7610-mergetool.sh.

> * make difftool read 'merge.guitool' as a fallback, in accordance to the
>   manpage for difftool: "git difftool falls back to git mergetool
>   config variables when the difftool equivalents have not been defined"

I did not spot this change in the code.

Nonetheless, this should be split off as a separate patch, and
tests should be added.

> * add guitool-related completions

This should be split off as a separate patch as well.

Generally, 3 bullet points suggests there should be 3 patches in
this series.

Thanks,
-- 
David

Re: [PATCH] tag: add a config option for setting --annotate by default

[David Aguilar]

On Fri, Feb 03, 2017 at 09:02:47PM -0800, Junio C Hamano wrote:
> David Aguilar <dav...@gmail.com> writes:
> 
> > Make it easier for users to remember to annotate their tags.
> > Allow setting the default value for "--annotate" via the "tag.annotate"
> > configuration variable.
> >
> > Signed-off-by: David Aguilar <dav...@gmail.com>
> > ---
> 
> I do not care too strongly about this, but I need to point out that
> this will have fallout to tools and scripts.  E.g. if you have this
> configured and try to create a new tag in gitk, wouldn't this part
> 
>   if {$msg != {}} {
>   exec git tag -a -m $msg $tag $id
>   } else {
>   exec git tag $tag $id
>   }
> 
> try to open an editor somehow to get the message even when $msg is
> an empty string?  I think the same problem already exists for the
> tag.forceSignAnnotated variable we already have added, though.

That's true.  I should have put "RFC" in the subject line.
Let's drop this patch unless there's others that find it useful.

How do you feel about a patch to add "git merge --signoff", for
consistency with "git commit"?

The rationale is that there might be situations (evil merges, or
even regular merges depending on the project) where someone
might want to signoff on their merges.
-- 
David

[PATCH] tag: add a config option for setting --annotate by default

[David Aguilar]

Make it easier for users to remember to annotate their tags.
Allow setting the default value for "--annotate" via the "tag.annotate"
configuration variable.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 Documentation/config.txt |  5 +
 builtin/tag.c| 15 ---
 t/t7004-tag.sh   | 23 +++
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..0d562b97e9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2945,6 +2945,11 @@ submodule.alternateErrorStrategy
as computed via `submodule.alternateLocation`. Possible values are
`ignore`, `info`, `die`. Default is `die`.
 
+tag.annotate::
+   A boolean to specify whether annotated tags should be created by
+   default.  If `--no-annotate` is specified on the command line,
+   it takes precedence over this option.
+
 tag.forceSignAnnotated::
A boolean to specify whether annotated tags created should be GPG 
signed.
If `--annotate` is specified on the command line, it takes
diff --git a/builtin/tag.c b/builtin/tag.c
index 73df728114..1cf9bb73ad 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -29,6 +29,7 @@ static const char * const git_tag_usage[] = {
 };
 
 static unsigned int colopts;
+static int force_annotate;
 static int force_sign_annotate;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, 
const char *format)
@@ -161,6 +162,10 @@ static int git_tag_config(const char *var, const char 
*value, void *cb)
status = git_gpg_config(var, value, cb);
if (status)
return status;
+   if (!strcmp(var, "tag.annotate")) {
+   force_annotate = git_config_bool(var, value);
+   return 0;
+   }
if (!strcmp(var, "tag.forcesignannotated")) {
force_sign_annotate = git_config_bool(var, value);
return 0;
@@ -326,7 +331,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct create_tag_options opt;
char *cleanup_arg = NULL;
int create_reflog = 0;
-   int annotate = 0, force = 0;
+   int annotate = -1, force = 0;
int cmdmode = 0, create_tag_object = 0;
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
@@ -387,11 +392,15 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
opt.sign = 1;
set_signing_key(keyid);
}
-   create_tag_object = (opt.sign || annotate || msg.given || msgfile);
 
if (argc == 0 && !cmdmode)
cmdmode = 'l';
 
+   if (force_annotate && !cmdmode && annotate == -1)
+   annotate = 1;
+
+   create_tag_object = (opt.sign || annotate > 0 || msg.given || msgfile);
+
if ((create_tag_object || force) && (cmdmode != 0))
usage_with_options(git_tag_usage, options);
 
@@ -478,7 +487,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("Invalid cleanup mode %s"), cleanup_arg);
 
if (create_tag_object) {
-   if (force_sign_annotate && !annotate)
+   if (force_sign_annotate && annotate == -1)
opt.sign = 1;
create_tag(object, tag, , , prev, object);
}
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 1cfa8a21d2..5ba52b57dd 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -754,6 +754,29 @@ echo from a fake editor.
 EOF
 chmod +x fakeeditor
 
+get_tag_header config-annotate $commit commit $time >expect
+./fakeeditor >>expect
+test_expect_success 'tag.annotate creates annotated tags' '
+   test_config tag.annotate true &&
+   GIT_EDITOR=./fakeeditor git tag config-annotate &&
+   get_tag_msg config-annotate >actual &&
+   test_cmp expect actual
+'
+test_expect_success 'tag --no-annotate overrides tag.annotate=true config' '
+   test_config tag.annotate true &&
+   GIT_EDITOR=false git tag --no-annotate cli-override-tag-annotate &&
+   tag_exists cli-override-tag-annotate
+'
+
+get_tag_header config-no-annotate $commit commit $time >expect
+./fakeeditor >>expect
+test_expect_success 'tag --annotate overrides tag.annotate=false config' '
+   test_config tag.annotate false &&
+   GIT_EDITOR=./fakeeditor git tag --annotate config-no-annotate &&
+   get_tag_msg config-no-annotate >actual &&
+   test_cmp expect actual
+'
+
 get_tag_header implied-sign $commit commit $time >expect
 ./fakeeditor >>expect
 echo '-BEGIN PGP SIGNATURE-' >>expect
-- 
2.11.0.486.gcc949b6e67.dirty

Re: mergetool and difftool inconsistency?

[David Aguilar]

On Wed, Jan 25, 2017 at 06:58:10PM -0800, Denton Liu wrote:
> Hello all,
> 
> I was wondering if there is any reason why 'git difftool' accepts the
> '-g|--gui' whereas 'git mergetool' does not have an option to accept
> that flag. Please let me know if this is intentional, otherwise I can
> write up a patch to add the GUI flag to 'mergetool'.
> 
> Thanks!

Hello,

The difference was not intentional.  The "--gui" feature was
added to difftool because that's where the feature was
originally requested, but there's no reason why mergetool
couldn't have a similar option.

cheers,
-- 
David

Re: [PATCH] difftool.c: mark a file-local symbol with static

[David Aguilar]

On Thu, Dec 01, 2016 at 01:50:57PM -0500, Jeff King wrote:
> On Thu, Dec 01, 2016 at 10:20:50AM -0800, Junio C Hamano wrote:
> > I also still think that any_printf_like_function("%s", "") looks
> > silly.  I know that we've already started moving in that direction
> > and we stopped at a place we do not want to be in, but perhaps it
> > was a mistake to move in that direction in the first place.  I am
> > tempted to say we should do something like the attached, but that
> > may not fly well, as I suspect that -Wno-format-zero-length may be a
> > lot more exotic than the -Wall compiler option.
> 
> Yeah, I think portability concerns are what caused us not to go down
> that road in the first place.
> makes it harder to disable if your compiler doesn't like it.
> 
> > An obvious second
> > best option would be to drop -Wall from the "everybody" CFLAGS and
> > move it to DEVELOPER_CFLAGS instead.
> 
> Yeah, though that doesn't help the example above.
> 
> As ugly as warning("%s", "") is, I think it may be the thing that annoys
> the smallest number of people.
> 
> -Peff

How about using warning(" ") instead?

For difftool.c specifically, the following is a fine solution,
and doesn't require that we change our warning flags just for
this one file.
-- 
David

--- 8< ---
>From 28bdc381202ced35399cfdf4899a019bc7a0 Mon Sep 17 00:00:00 2001
From: David Aguilar <dav...@gmail.com>
Date: Sat, 21 Jan 2017 21:21:09 -0800
Subject: [PATCH] difftool: avoid zero-length format string

The purpose of the warning("") line is to add an empty line to
improve the readability of the message.

Use warning(" ") instead to avoid zero-length format string
warnings while retaining the desired behavior.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 builtin/difftool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 42ad9e804a..d9f8ada291 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
warning(_("both files modified: '%s' and 
'%s'."),
wtdir.buf, rdir.buf);
warning(_("working tree file has been left."));
-   warning("");
+   warning(" ");
err = 1;
} else if (unlink(wtdir.buf) ||
   copy_file(wtdir.buf, rdir.buf, st.st_mode))
-- 
2.11.0.747.g28bdc38120

[PATCH] gitk: remove translated message from comments

[David Aguilar]

"make update-po" fails because a previously untranslated string
has now been translated:

Updating po/sv.po
po/sv.po:1388: duplicate message definition...
po/sv.po:380: ...this is the location of the first definition

Remove the duplicate message definition.

Reported-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
 po/sv.po | 15 ---
 1 file changed, 15 deletions(-)

diff --git a/po/sv.po b/po/sv.po
index 32fc752..2a06fe5 100644
--- a/po/sv.po
+++ b/po/sv.po
@@ -1385,21 +1385,6 @@ msgstr "Felaktiga argument till gitk:"
 #~ msgid "mc"
 #~ msgstr "mc"
 
-#~ msgid ""
-#~ "\n"
-#~ "Gitk - a commit viewer for git\n"
-#~ "\n"
-#~ "Copyright © 2005-2016 Paul Mackerras\n"
-#~ "\n"
-#~ "Use and redistribute under the terms of the GNU General Public License"
-#~ msgstr ""
-#~ "\n"
-#~ "Gitk - en incheckningsvisare för git\n"
-#~ "\n"
-#~ "Copyright © 2005-2016 Paul Mackerras\n"
-#~ "\n"
-#~ "Använd och vidareförmedla enligt villkoren i GNU General Public License"
-
 #~ msgid "next"
 #~ msgstr "nästa"
 
-- 
2.11.0.536.gaf746e49c2

gitk pull request // was: Re: gitk: "lime" color incompatible with older Tk versions

[David Aguilar]

On Fri, Jan 13, 2017 at 03:20:43AM -0800, David Aguilar wrote:
> 
> Ping.. it would be nice to get this patch applied.

Sorry for the noise, and thank you Paul for the fix.
This was already fixed by Paul in gitk@22a713c72df.

I'm sure Junio will merge gitk.git into git.git soon enough so I
can sit tight until then, but while I'm here I might as well
send out a pull request:

The following changes since commit 22a713c72df8b6799c59287c50cee44c4a6db51e:

  gitk: Follow themed bgcolor in help dialogs (2016-03-19 14:12:21 +1100)

are available in the git repository at:

  git://ozlabs.org/~paulus/gitk.git 

for you to fetch changes up to fbf426478e540f4737860dae622603cc0daba3d2:

  gitk: Update copyright notice to 2016 (2016-12-12 20:46:42 +1100)


Markus Hitter (3):
  gitk: Turn off undo manager in the text widget
  gitk: Remove closed file descriptors from $blobdifffd
  gitk: Clear array 'commitinfo' on reload

Paul Mackerras (2):
  gitk: Use explicit RGB green instead of "lime"
  gitk: Update copyright notice to 2016

Rogier Goossens (3):
  gitk: Add a 'rename' option to the branch context menu
  gitk: Allow checking out a remote branch
  gitk: Include commit title in branch dialog

Satoshi Yasushima (1):
  gitk: Fix Japanese translation for "marked commit"

Stefan Dotterweich (1):
  gitk: Fix missing commits when using -S or -G

Vasco Almeida (2):
  gitk: Makefile: create install bin directory
  gitk: Add Portuguese translation

 Makefile|1 +
 gitk|  166 +--
 po/bg.po|4 +-
 po/ca.po|6 +-
 po/de.po|4 +-
 po/es.po|4 +-
 po/fr.po|4 +-
 po/hu.po|4 +-
 po/it.po|4 +-
 po/ja.po|   13 +-
 po/pt_br.po |4 +-
 po/pt_pt.po | 1376 +++
 po/ru.po|4 +-
 po/sv.po|8 +-
 po/vi.po|4 +-
 15 files changed, 1549 insertions(+), 57 deletions(-)
 create mode 100644 po/pt_pt.po

Thanks,
-- 
David

Re: gitk: "lime" color incompatible with older Tk versions

[David Aguilar]

On Mon, May 02, 2016 at 09:20:43AM -0700, Stefan Beller wrote:
> + Paul Mackerras, who maintains gitk
> 
> On Sun, May 1, 2016 at 10:03 AM, Andrew Janke  wrote:
> > Hi, git folks,
> >
> > I'm having trouble running gitk on Mac OS X 10.9.5. The gitk program uses
> > the color "lime", which is not present in older versions of Tk, apparently
> > including the Tk 8.5 which ships with 10.9.

Ping.. it would be nice to get this patch applied.
I can verify that gitk on Mac OS X 10.11 also has this problem.
gitk is usually pretty good about backwards-compatibility.

> > This compatibility problem was noted before back in 2012, in
> > http://www.mail-archive.com/git%40vger.kernel.org/msg14496.html.
> >
> > Would you consider switching from lime to a hex value color, for
> > compatibility with users of older versions of Tk? A patch to do so is below;
> > only the file gitk-git/gitk needs to be changed.

I can recreate and resend this patch if needed; it's simply:
:%s/lime/"#99FF00"/g

Would a re-roll of this patch be accepted, or is it not worth
bothering?

Google for "gitk lime" to get a taste for some of the fallout
caused by this problem.

The fact that multiple pages, with different OS's, have examples
of users stumbling over this change is a good hint that it's
worth fixing.

Thoughts?
-- 
David

Re: [PATCH 0/4] fix mergetool+rerere+subdir regression

[David Aguilar]

On Tue, Jan 03, 2017 at 07:50:38PM -0500, Richard Hansen wrote:
> If rerere is enabled, no pathnames are given, and mergetool is run
> from a subdirectory, mergetool always prints "No files need merging".
> Fix the bug.
> 
> This regression was introduced in
> 57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
> 
> Richard Hansen (4):
>   t7610: update branch names to match test number
>   t7610: make tests more independent and debuggable
>   t7610: add test case for rerere+mergetool+subdir bug
>   mergetool: fix running in subdir when rerere enabled
> 
>  git-mergetool.sh |   1 +
>  t/t7610-mergetool.sh | 132 
> ++-
>  2 files changed, 90 insertions(+), 43 deletions(-)

Thanks for finding these, this reminds me very much of the
recent fixes that had to be done to difftool.

I tested this so,

Acked-by: David Aguilar <dav...@gmail.com>
-- 
David

Re: [PATCH 2/2] mergetools/tortoisemerge: simplify can_diff() by using "false"

[David Aguilar]

On Sat, Dec 10, 2016 at 09:15:34AM +0100, Johannes Sixt wrote:
> Am 10.12.2016 um 04:21 schrieb David Aguilar:
> > Signed-off-by: David Aguilar <dav...@gmail.com>
> > ---
> > This patch builds upon da/mergetool-trust-exit-code
> > 
> >  mergetools/tortoisemerge | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
> > index d7ab666a59..9067d8a4e5 100644
> > --- a/mergetools/tortoisemerge
> > +++ b/mergetools/tortoisemerge
> > @@ -1,5 +1,5 @@
> >  can_diff () {
> > -   return 1
> > +   false
> >  }
> 
> Why is this a simplification?
> 
> My concern is that 'false' is not necessarily a shell built-in. Then this is
> actually a pessimization.

The "simplification" is semantic only.

Motivation: if someone reads the implementation of can_diff()
and it says "false" then that communicates intent moreso than
reading "return 1", which a programmer unfamiliar with shell
conventions might misinterpret as boolean "true".

I care less about semantics then I do about making things better
for Windows, so we can forget about these two patches.
-- 
David

[PATCH 1/2] mergetools/kompare: simplify can_merge() by using "false"

[David Aguilar]

Signed-off-by: David Aguilar <dav...@gmail.com>
---
This patch builds upon da/mergetool-trust-exit-code

 mergetools/kompare | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mergetools/kompare b/mergetools/kompare
index e8c0bfa678..321022500b 100644
--- a/mergetools/kompare
+++ b/mergetools/kompare
@@ -1,5 +1,5 @@
 can_merge () {
-   return 1
+   false
 }
 
 diff_cmd () {
-- 
2.11.0.27.gdeff8c7

[PATCH 2/2] mergetools/tortoisemerge: simplify can_diff() by using "false"

[David Aguilar]

Signed-off-by: David Aguilar <dav...@gmail.com>
---
This patch builds upon da/mergetool-trust-exit-code

 mergetools/tortoisemerge | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index d7ab666a59..9067d8a4e5 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -1,5 +1,5 @@
 can_diff () {
-   return 1
+   false
 }
 
 merge_cmd () {
-- 
2.11.0.27.gdeff8c7

[PATCH] mergetools: fix xxdiff hotkeys

[David Aguilar]

xxdiff was using a mix of "Ctrl-" and "Ctrl+" hotkeys.
The dashed "-" form is not accepted by newer xxdiff versions.
Use the plus "+" form only.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
This patch is based on top of da/mergetool-diff-order

 mergetools/xxdiff | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mergetools/xxdiff b/mergetools/xxdiff
index e284811ff2..ce5b8e9f29 100644
--- a/mergetools/xxdiff
+++ b/mergetools/xxdiff
@@ -1,7 +1,7 @@
 diff_cmd () {
"$merge_tool_path" \
-R 'Accel.Search: "Ctrl+F"' \
-   -R 'Accel.SearchForward: "Ctrl-G"' \
+   -R 'Accel.SearchForward: "Ctrl+G"' \
"$LOCAL" "$REMOTE"
 }
 
@@ -9,15 +9,15 @@ merge_cmd () {
if $base_present
then
"$merge_tool_path" -X --show-merged-pane \
-   -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+   -R 'Accel.SaveAsMerged: "Ctrl+S"' \
-R 'Accel.Search: "Ctrl+F"' \
-   -R 'Accel.SearchForward: "Ctrl-G"' \
+   -R 'Accel.SearchForward: "Ctrl+G"' \
--merged-file "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
else
"$merge_tool_path" -X $extra \
-   -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+   -R 'Accel.SaveAsMerged: "Ctrl+S"' \
-R 'Accel.Search: "Ctrl+F"' \
-   -R 'Accel.SearchForward: "Ctrl-G"' \
+   -R 'Accel.SearchForward: "Ctrl+G"' \
--merged-file "$MERGED" "$LOCAL" "$REMOTE"
fi
 }
-- 
2.11.0.rc3.8.gaca8798.dirty

Re: [PATCH 2/3] difftool: chdir as early as possible

[David Aguilar]

On Fri, Dec 09, 2016 at 03:02:09PM -0800, Junio C Hamano wrote:
> David Aguilar <dav...@gmail.com> writes:
> 
> > @@ -182,10 +188,6 @@ EOF
> > }
> > }
> >  
> > -   # Go to the root of the worktree so that the left index files
> > -   # are properly setup -- the index is toplevel-relative.
> > -   chdir($workdir);
> > -
> > # Setup temp directories
> > my $tmpdir = tempdir('git-difftool.X', CLEANUP => 0, TMPDIR => 1);
> > my $ldir = "$tmpdir/left";
> 
> What codebase are you basing your work on?  I do not see these
> removed four lines in my tree, so it seems that the patch is fixing
> up some other fix I do not yet have.

Sorry about that.

I forgot to mention that this is based on the patches I recently
sent, which were in the "pu" branch.  The whats-cooking report
mentioned that they'll be merged to "next", so they might be
there already too.

The patch this was based upon:

difftool: fix dir-diff index creation when in a subdirectory
-- 
David

[PATCH 3/3] difftool: rename variables for consistency

[David Aguilar]

Always call the list of files @files.
Always call the worktree $worktree.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-difftool.perl | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 99b03949bf..4e4f5d8138 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -100,7 +100,7 @@ sub changed_files
 
 sub setup_dir_diff
 {
-   my ($workdir, $symlinks) = @_;
+   my ($worktree, $symlinks) = @_;
my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
my $diffrtn = Git::command_oneline(@gitargs);
exit(0) unless defined($diffrtn);
@@ -109,7 +109,7 @@ sub setup_dir_diff
# changed files.  The paths returned by diff --raw are relative to the
# top-level of the repository, but we defer changing directories so
# that @ARGV can perform pathspec limiting in the current directory.
-   chdir($workdir);
+   chdir($worktree);
 
# Build index info for left and right sides of the diff
my $submodule_mode = '16';
@@ -121,7 +121,7 @@ sub setup_dir_diff
my $wtindex = '';
my %submodule;
my %symlink;
-   my @working_tree = ();
+   my @files = ();
my %working_tree_dups = ();
my @rawdiff = split('\0', $diffrtn);
 
@@ -173,14 +173,14 @@ EOF
}
 
if ($rmode ne $null_mode) {
-   # Avoid duplicate working_tree entries
+   # Avoid duplicate entries
if ($working_tree_dups{$dst_path}++) {
next;
}
my ($use, $wt_sha1) =
use_wt_file($dst_path, $rsha1);
if ($use) {
-   push @working_tree, $dst_path;
+   push @files, $dst_path;
$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
} else {
$rindex .= "$rmode $rsha1\t$dst_path\0";
@@ -227,14 +227,14 @@ EOF
 
# Changes in the working tree need special treatment since they are
# not part of the index.
-   for my $file (@working_tree) {
+   for my $file (@files) {
my $dir = dirname($file);
unless (-d "$rdir/$dir") {
mkpath("$rdir/$dir") or
exit_cleanup($tmpdir, 1);
}
if ($symlinks) {
-   symlink("$workdir/$file", "$rdir/$file") or
+   symlink("$worktree/$file", "$rdir/$file") or
exit_cleanup($tmpdir, 1);
} else {
copy($file, "$rdir/$file") or
@@ -278,7 +278,7 @@ EOF
exit_cleanup($tmpdir, 1) if not $ok;
}
 
-   return ($ldir, $rdir, $tmpdir, @working_tree);
+   return ($ldir, $rdir, $tmpdir, @files);
 }
 
 sub write_to_file
@@ -388,9 +388,9 @@ sub dir_diff
my $error = 0;
my $repo = Git->repository();
my $repo_path = $repo->repo_path();
-   my $workdir = $repo->wc_path();
-   $workdir =~ s|/$||; # Avoid double slashes in symlink targets
-   my ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks);
+   my $worktree = $repo->wc_path();
+   $worktree =~ s|/$||; # Avoid double slashes in symlink targets
+   my ($a, $b, $tmpdir, @files) = setup_dir_diff($worktree, $symlinks);
 
if (defined($extcmd)) {
$rc = system($extcmd, $a, $b);
@@ -411,13 +411,13 @@ sub dir_diff
my %tmp_modified;
my $indices_loaded = 0;
 
-   for my $file (@worktree) {
+   for my $file (@files) {
next if $symlinks && -l "$b/$file";
next if ! -f "$b/$file";
 
if (!$indices_loaded) {
%wt_modified = changed_files(
-   $repo_path, "$tmpdir/wtindex", $workdir);
+   $repo_path, "$tmpdir/wtindex", $worktree);
%tmp_modified = changed_files(
$repo_path, "$tmpdir/wtindex", $b);
$indices_loaded = 1;
@@ -425,7 +425,7 @@ sub dir_diff
 
if (exists $wt_modified{$file} and exists $tmp_modified{$file}) 
{
my $errmsg = "warning: Both files modified: ";
-   $errmsg .= "'$workdir/$file' and '$b/$file'.\n";
+   $errmsg .= "'$worktree/$file' and '$b/$file'.\n";
$errmsg .= "warning: Working tree file has been 
left.\n";
$errmsg .= "warning:\n";
warn $errmsg;
-- 
2.11.0.26.gb65c994

[PATCH 2/3] difftool: chdir as early as possible

[David Aguilar]

Make difftool chdir to the top-level of the repository as soon as it can
so that we can simplify how paths are handled.  Replace construction of
absolute paths via string concatenation with relative paths wherever
possible.  The bulk of the code no longer needs to use absolute paths.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-difftool.perl | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 17c336321f..99b03949bf 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -59,14 +59,14 @@ sub exit_cleanup
 
 sub use_wt_file
 {
-   my ($workdir, $file, $sha1) = @_;
+   my ($file, $sha1) = @_;
my $null_sha1 = '0' x 40;
 
-   if (-l "$workdir/$file" || ! -e _) {
+   if (-l $file || ! -e _) {
return (0, $null_sha1);
}
 
-   my $wt_sha1 = Git::command_oneline('hash-object', "$workdir/$file");
+   my $wt_sha1 = Git::command_oneline('hash-object', $file);
my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
return ($use, $wt_sha1);
 }
@@ -105,6 +105,12 @@ sub setup_dir_diff
my $diffrtn = Git::command_oneline(@gitargs);
exit(0) unless defined($diffrtn);
 
+   # Go to the root of the worktree now that we've captured the list of
+   # changed files.  The paths returned by diff --raw are relative to the
+   # top-level of the repository, but we defer changing directories so
+   # that @ARGV can perform pathspec limiting in the current directory.
+   chdir($workdir);
+
# Build index info for left and right sides of the diff
my $submodule_mode = '16';
my $symlink_mode = '12';
@@ -172,7 +178,7 @@ EOF
next;
}
my ($use, $wt_sha1) =
-   use_wt_file($workdir, $dst_path, $rsha1);
+   use_wt_file($dst_path, $rsha1);
if ($use) {
push @working_tree, $dst_path;
$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
@@ -182,10 +188,6 @@ EOF
}
}
 
-   # Go to the root of the worktree so that the left index files
-   # are properly setup -- the index is toplevel-relative.
-   chdir($workdir);
-
# Setup temp directories
my $tmpdir = tempdir('git-difftool.X', CLEANUP => 0, TMPDIR => 1);
my $ldir = "$tmpdir/left";
@@ -235,10 +237,10 @@ EOF
symlink("$workdir/$file", "$rdir/$file") or
exit_cleanup($tmpdir, 1);
} else {
-   copy("$workdir/$file", "$rdir/$file") or
+   copy($file, "$rdir/$file") or
exit_cleanup($tmpdir, 1);
 
-   my $mode = stat("$workdir/$file")->mode;
+   my $mode = stat($file)->mode;
chmod($mode, "$rdir/$file") or
exit_cleanup($tmpdir, 1);
}
@@ -430,10 +432,10 @@ sub dir_diff
$error = 1;
} elsif (exists $tmp_modified{$file}) {
my $mode = stat("$b/$file")->mode;
-   copy("$b/$file", "$workdir/$file") or
+   copy("$b/$file", $file) or
exit_cleanup($tmpdir, 1);
 
-   chmod($mode, "$workdir/$file") or
+   chmod($mode, $file) or
exit_cleanup($tmpdir, 1);
}
}
-- 
2.11.0.26.gb65c994

[PATCH 1/3] difftool: sanitize $workdir as early as possible

[David Aguilar]

The double-slash fixup on the $workdir variable was being
performed just-in-time to avoid double-slashes in symlink
targets, but the rest of the code was silently using paths with
embedded "//" in them.

A recent user-reported error message contained double-slashes.
Eliminate the issue by sanitizing inputs as soon as they arrive.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-difftool.perl | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 959822d5f3..17c336321f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -224,9 +224,7 @@ EOF
delete($ENV{GIT_INDEX_FILE});
 
# Changes in the working tree need special treatment since they are
-   # not part of the index. Remove any trailing slash from $workdir
-   # before starting to avoid double slashes in symlink targets.
-   $workdir =~ s|/$||;
+   # not part of the index.
for my $file (@working_tree) {
my $dir = dirname($file);
unless (-d "$rdir/$dir") {
@@ -389,6 +387,7 @@ sub dir_diff
my $repo = Git->repository();
my $repo_path = $repo->repo_path();
my $workdir = $repo->wc_path();
+   $workdir =~ s|/$||; # Avoid double slashes in symlink targets
my ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks);
 
if (defined($extcmd)) {
-- 
2.11.0.26.gb65c994

[PATCH v2] difftool: fix dir-diff index creation when in a subdirectory

[David Aguilar]

9ec26e7977 (difftool: fix argument handling in subdirs, 2016-07-18)
corrected how path arguments are handled in a subdirectory, but
it introduced a regression in how entries outside of the
subdirectory are handled by dir-diff.

When preparing the right-side of the diff we only include the
changed paths in the temporary area.

The left side of the diff is constructed from a temporary
index that is built from the same set of changed files, but it
was being constructed from within the subdirectory.  This is a
problem because the indexed paths are toplevel-relative, and
thus they were not getting added to the index.

Teach difftool to chdir to the toplevel of the repository before
preparing its temporary indexes.  This ensures that all of the
toplevel-relative paths are valid.

Add test cases to more thoroughly exercise this scenario.

Reported-by: Frank Becker <f...@mooflu.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
Changes since v1:

- Improved commit message thanks to Johannes Schindelin
  ("git whatis" was used to describe the referenced commit)

- Add more tests for edge cases that are properly handled
  but were not being tested.

 git-difftool.perl   |  4 
 t/t7800-difftool.sh | 44 +---
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index a5790d03a0..959822d5f3 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -182,6 +182,10 @@ EOF
}
}
 
+   # Go to the root of the worktree so that the left index files
+   # are properly setup -- the index is toplevel-relative.
+   chdir($workdir);
+
# Setup temp directories
my $tmpdir = tempdir('git-difftool.X', CLEANUP => 0, TMPDIR => 1);
my $ldir = "$tmpdir/left";
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461a..99d4123461 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -374,6 +374,7 @@ test_expect_success PERL 'setup change in subdirectory' '
echo master >sub/sub &&
git add sub/sub &&
git commit -m "added sub/sub" &&
+   git tag v1 &&
echo test >>file &&
echo test >>sub/sub &&
git add file sub/sub &&
@@ -409,12 +410,49 @@ run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
grep file output
 '
 
-run_dir_diff_test 'difftool --dir-diff from subdirectory' '
+run_dir_diff_test 'difftool --dir-diff branch from subdirectory' '
(
cd sub &&
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
-   grep sub output &&
-   grep file output
+   # "sub" must only exist in "right"
+   # "file" and "file2" must be listed in both "left" and "right"
+   test "1" = $(grep sub output | wc -l) &&
+   test "2" = $(grep file"$" output | wc -l) &&
+   test "2" = $(grep file2 output | wc -l)
+   )
+'
+
+run_dir_diff_test 'difftool --dir-diff v1 from subdirectory' '
+   (
+   cd sub &&
+   git difftool --dir-diff $symlinks --extcmd ls v1 >output &&
+   # "sub" and "file" exist in both v1 and HEAD.
+   # "file2" is unchanged.
+   test "2" = $(grep sub output | wc -l) &&
+   test "2" = $(grep file output | wc -l) &&
+   test "0" = $(grep file2 output | wc -l)
+   )
+'
+
+run_dir_diff_test 'difftool --dir-diff branch from subdirectory w/ pathspec' '
+   (
+   cd sub &&
+   git difftool --dir-diff $symlinks --extcmd ls branch -- 
.>output &&
+   # "sub" only exists in "right"
+   # "file" and "file2" must not be listed
+   test "1" = $(grep sub output | wc -l) &&
+   test "0" = $(grep file output | wc -l)
+   )
+'
+
+run_dir_diff_test 'difftool --dir-diff v1 from subdirectory w/ pathspec' '
+   (
+   cd sub &&
+   git difftool --dir-diff $symlinks --extcmd ls v1 -- .>output &&
+   # "sub" exists in v1 and HEAD
+   # "file" is filtered out by the pathspec
+   test "2" = $(grep sub output | wc -l) &&
+   test "0" = $(grep file output | wc -l)
)
 '
 
-- 
2.11.0.1.g7697df0

[PATCH] difftool: fix dir-diff index creation when in a subdirectory

[David Aguilar]

9ec26e797781239b36ebccb87c590e5778358007 corrected how path arguments
are handled in a subdirectory, but it introduced a regression in how
entries outside of the subdirectory are handled by dir-diff.

When preparing the right-side of the diff we only include the
changed paths in the temporary area.

The left side of the diff is constructed from a temporary
index that is built from the same set of changed files, but it
was being constructed from within the subdirectory.  This is a
problem because the indexed paths are toplevel-relative, and
thus they were not getting added to the index.

Teach difftool to chdir to the toplevel of the repository before
preparing its temporary indexes.  This ensures that all of the
toplevel-relative paths are valid.

Adjust the test cases to more thoroughly exercise this scenario.

Reported-by: Frank Becker <f...@mooflu.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
I figured I'd send this as a proper patch.
The patch contents have not changed since the original in-thread
patch, but the commit message was modified slightly.

 git-difftool.perl   | 4 
 t/t7800-difftool.sh | 7 +--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index a5790d03a..959822d5f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -182,6 +182,10 @@ EOF
}
}
 
+   # Go to the root of the worktree so that the left index files
+   # are properly setup -- the index is toplevel-relative.
+   chdir($workdir);
+
# Setup temp directories
my $tmpdir = tempdir('git-difftool.X', CLEANUP => 0, TMPDIR => 1);
my $ldir = "$tmpdir/left";
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461..caab4b5ca 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -413,8 +413,11 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '
(
cd sub &&
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
-   grep sub output &&
-   grep file output
+   # "sub" must only exist in "right"
+   # "file" and "file2" must be listed in both "left" and "right"
+   test "1" = $(grep sub output | wc -l) &&
+   test "2" = $(grep file"$" output | wc -l) &&
+   test "2" = $(grep file2 output | wc -l)
)
 '
 
-- 
2.11.0

Re: Error after calling git difftool -d with

[David Aguilar]

On Fri, Dec 02, 2016 at 05:05:06PM +0100, Johannes Schindelin wrote:
> Hi Peter,
> 
> On Fri, 2 Dec 2016, P. Duijst wrote:
> 
> > Incase filenames are used with a quote ' or a bracket [  (and maybe some 
> > more
> > characters), git "diff" and "difftool -y" works fine, but git *difftool 
> > **-d*
> > gives the next error message:
> > 
> >peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> >$ git diff
> >diff --git a/Test ''inch.txt b/Test ''inch.txt
> >index dbff793..41f3257 100644
> >--- a/Test ''inch.txt
> >+++ b/Test ''inch.txt
> >@@ -1 +1,3 @@
> >+
> >+ddd
> >  Test error in simple repository
> >warning: LF will be replaced by CRLF in Test ''inch.txt.
> >The file will have its original line endings in your working directory.
> > 
> >peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> >*$ git difftool -d*
> >*fatal: Cannot open '/d/Dev/test//Test ''inch.txt': No such file or
> >directory*
> >*hash-object /d/Dev/test//Test ''inch.txt: command returned error: 128*
> > 
> >peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> >$
> > 
> > 
> > This issue is inside V2.10.x and V2.11.0.
> > V2.9.0 is working correctly...
> 
> You say v2.11.0, but did you also try the new, experimental builtin
> difftool? You can test without reinstalling:
> 
>   git -c difftool.useBuiltin=true difftool -d ...

FWIW, I verified that this problem does not manifest itself on
Linux, using the current scripted difftool.

Peter, what actual diff tool are you using?

Since these filenames work fine with "difftool -d" on Linux, it
suggests that this is either a tool-specific issue, or an issue
related to unix-to-windows path translation.
-- 
David

Re: difftool -d not populating left correctly when not in git root

[David Aguilar]

:17 -0800
Subject: [PATCH] difftool: properly handle being launched from a subdirectory

9ec26e797781239b36ebccb87c590e5778358007 corrected how path arguments
are handled in a subdirectory, but it introduced a regression in how
entries outside of the subdirectory are handled by the dir-diff.

When preparing the right-side of the diff we only construct the parts
that changed.

When constructing the left side we construct an index, but we were
constructing it relative to the subdirectory, and thus it ends up empty
because we are in a subdirectory and the paths are incorrect.

Teach difftool to chdir to the toplevel of the repository before
preparing its temporary indexes.  This ensures that all of the
toplevel-relative paths are valid.

Add a test case to exercise this use case.

Reported-by: Frank Becker <f...@mooflu.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-difftool.perl   | 4 
 t/t7800-difftool.sh | 7 +--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index a5790d03a..959822d5f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -182,6 +182,10 @@ EOF
}
}
 
+   # Go to the root of the worktree so that the left index files
+   # are properly setup -- the index is toplevel-relative.
+   chdir($workdir);
+
# Setup temp directories
my $tmpdir = tempdir('git-difftool.X', CLEANUP => 0, TMPDIR => 1);
my $ldir = "$tmpdir/left";
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461..caab4b5ca 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -413,8 +413,11 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '
(
cd sub &&
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
-   grep sub output &&
-   grep file output
+   # "sub" must only exist in "right"
+   # "file" and "file2" must be listed in both "left" and "right"
+   test "1" = $(grep sub output | wc -l) &&
+   test "2" = $(grep file"$" output | wc -l) &&
+   test "2" = $(grep file2 output | wc -l)
)
 '
 
-- 
2.11.0.dirty

Re: [BUG] git gui can't commit multiple files

[David Aguilar]

On Sun, Dec 04, 2016 at 05:36:46PM +0100, Timon wrote:
> This is a regression in git 2.11.0 (version 2.10.2 is fine).
> 
> In git-gui I select multiple files in the Unstaged Changes (using
> shift+click) and press ctrl+t to stage them. Then only one files gets
> staged instead of all of the selected files.
> The same happens when unstaging files.
> 
> Git-cola also exhibits the same behavior. Although there I could stage
> multiple files if I used a popup menu instead of the keyboard shortcut
> (I'm guessing it goes through a different code path?).

Can you elaborate a bit?

I just tested git-cola with Git 2.11 and it worked fine for me.
I selected several files and used the Ctrl+s hotkey to stage the
selected files.  They all got staged.

If you have a test repo, or reproduction recipe, I'd be curious
to try it out.
-- 
David

Re: CVSImport - spaces in CVS path

[David Aguilar]

On Wed, Nov 30, 2016 at 01:56:35PM -0700, Yojoa wrote:
> I'm in the process of moving an entire collection of cvs modules into git.
> I'm working in Mac Yosemite. Everything is working fine except for one
> thing. A couple of the CVS modules have spaces in the paths. Below is what
> my command line looks like. When the path has spaces I've tried putting it
> in single and double quotes and using escape characters.  None of that
> matters. I always get a message that it can't read dir/dir/path and then
> messages that it can't find the modules "with" or "spaces".  
> 
> git cvsimport -v -d :pserver:MYLOGIN:/usr/local/cvsroot/
> dir/dir/PathWithNoSpaces/dir
> 
> git cvsimport -v -d :pserver:MYLOGIN:/usr/local/cvsroot/ dir/dir/path with
> spaces/dir

Try cvs-fast-export + cvsconvert.

http://www.catb.org/~esr/cvs-fast-export/

http://www.catb.org/~esr/cvs-fast-export/cvsconvert.html
-- 
David

[PATCH 1/2] mergetool: honor mergetool.$tool.trustExitCode for built-in tools

[David Aguilar]

Built-in merge tools contain a hard-coded assumption about
whether or not a tool's exit code can be trusted to determine
the success or failure of a merge.  Tools whose exit codes are
not trusted contain calls to check_unchanged() in their
merge_cmd() functions.

A problem with this is that the trustExitCode configuration is
not honored for built-in tools.

Teach built-in tools to honor the trustExitCode configuration.
Extend run_merge_cmd() so that it is responsible for calling
check_unchanged() when a tool's exit code cannot be trusted.
Remove check_unchanged() calls from scriptlets since they are no
longer responsible for calling it.

When no configuration is present, exit_code_trustable() is
checked to see whether the exit code should be trusted.
The default implementation returns false.

Tools whose exit codes can be trusted override
exit_code_trustable() to true.

Reported-by: Dun Peal <dunpea...@gmail.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-mergetool--lib.sh| 56 ++--
 mergetools/araxis|  2 --
 mergetools/bc|  2 --
 mergetools/codecompare   |  2 --
 mergetools/deltawalker   |  6 +-
 mergetools/diffmerge |  4 
 mergetools/diffuse   |  2 --
 mergetools/ecmerge   |  2 --
 mergetools/emerge|  4 
 mergetools/examdiff  |  2 --
 mergetools/kdiff3|  4 
 mergetools/kompare   |  4 
 mergetools/meld  |  3 +--
 mergetools/opendiff  |  2 --
 mergetools/p4merge   |  2 --
 mergetools/tkdiff|  4 
 mergetools/tortoisemerge |  2 --
 mergetools/vimdiff   |  2 --
 mergetools/winmerge  |  2 --
 mergetools/xxdiff|  2 --
 20 files changed, 71 insertions(+), 38 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9abd00be2..9a8b97a2a 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -125,16 +125,7 @@ setup_user_tool () {
}
 
merge_cmd () {
-   trust_exit_code=$(git config --bool \
-   "mergetool.$1.trustExitCode" || echo false)
-   if test "$trust_exit_code" = "false"
-   then
-   touch "$BACKUP"
-   ( eval $merge_tool_cmd )
-   check_unchanged
-   else
-   ( eval $merge_tool_cmd )
-   fi
+   ( eval $merge_tool_cmd )
}
 }
 
@@ -162,6 +153,28 @@ setup_tool () {
echo "$1"
}
 
+   # Most tools' exit codes cannot be trusted, so By default we ignore
+   # their exit code and check the merged file's modification time in
+   # check_unchanged() to determine whether or not the merge was
+   # successful.  The return value from run_merge_cmd, by default, is
+   # determined by check_unchanged().
+   #
+   # When a tool's exit code can be trusted then the return value from
+   # run_merge_cmd is simply the tool's exit code, and check_unchanged()
+   # is not called.
+   #
+   # The return value of exit_code_trustable() tells us whether or not we
+   # can trust the tool's exit code.
+   #
+   # User-defined and built-in tools default to false.
+   # Built-in tools advertise that their exit code is trustable by
+   # redefining exit_code_trustable() to true.
+
+   exit_code_trustable () {
+   false
+   }
+
+
if ! test -f "$MERGE_TOOLS_DIR/$tool"
then
setup_user_tool
@@ -197,6 +210,19 @@ get_merge_tool_cmd () {
fi
 }
 
+trust_exit_code () {
+   if git config --bool "mergetool.$1.trustExitCode"
+   then
+   :; # OK
+   elif exit_code_trustable
+   then
+   echo true
+   else
+   echo false
+   fi
+}
+
+
 # Entry point for running tools
 run_merge_tool () {
# If GIT_PREFIX is empty then we cannot use it in tools
@@ -225,7 +251,15 @@ run_diff_cmd () {
 
 # Run a either a configured or built-in merge tool
 run_merge_cmd () {
-   merge_cmd "$1"
+   mergetool_trust_exit_code=$(trust_exit_code "$1")
+   if test "$mergetool_trust_exit_code" = "true"
+   then
+   merge_cmd "$1"
+   else
+   touch "$BACKUP"
+   merge_cmd "$1"
+   check_unchanged
+   fi
 }
 
 list_merge_tool_candidates () {
diff --git a/mergetools/araxis b/mergetools/araxis
index 64f97c5e9..e2407b65b 100644
--- a/mergetools/araxis
+++ b/mergetools/araxis
@@ -3,7 +3,6 @@ diff_cmd () {
 }
 
 merge_cmd () {
-   touch "$BACKUP"
if $base_present
then
"$merge_tool_path" -wait -merge -3 -a1 \
@@ -12,7 +11,6 @@ merge_cmd () {
"$merge_tool_path" -wait -2

[PATCH 2/2] mergetools/vimdiff: trust Vim's exit code

[David Aguilar]

Allow vimdiff users to signal that they do not want to use the
result of a merge by exiting with ":cquit", which tells Vim to
exit with an error code.

This is better than the current behavior because it allows users
to directly flag that the merge is bad, using a standard Vim
feature, rather than relying on a timestamp heuristic that is
unforgiving to users that save in-progress merge files.

The original behavior can be restored by configuring
mergetool.vimdiff.trustExitCode to false.

Reported-by: Dun Peal <dunpea...@gmail.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
I've included anyone that has ever touched the vimdiff feature on the Cc:
list since I'm assuming that you use vimdiff.

This change is a slight change in default behavior when using
mergetool with vimdiff, but I think it's a better default overall.

 mergetools/vimdiff | 4 
 1 file changed, 4 insertions(+)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index a841ffdb4..10d86f3e1 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -42,3 +42,7 @@ translate_merge_tool_path() {
;;
esac
 }
+
+exit_code_trustable () {
+   true
+}
-- 
2.11.0.rc3.6.g2e567fd

Re: trustExitCode doesn't apply to vimdiff mergetool

[David Aguilar]

On Sun, Nov 27, 2016 at 05:45:38PM -0800, David Aguilar wrote:
> On Sun, Nov 27, 2016 at 11:55:59AM -0500, Jeff King wrote:
> > On Sun, Nov 27, 2016 at 08:46:40AM -0500, Dun Peal wrote:
> > 
> > > Ignoring a non-zero exit code from the merge tool, and assuming a
> > > successful merge in that case, seems like the wrong default behavior
> > > to me.
> > 
> > Yeah, I'm inclined to agree. But like I said, I'm not too familiar with
> > this area, so maybe there are subtle things I'm missing.
> 
> I think this may have been an oversight in how the
> trust-exit-code feature is implemented across builtins.
> 
> Right now, specific builtin tools could in theory opt-in to the
> feature, but I think it should be handled in a central place.
> For vimdiff, the exit code is not considered because the
> scriptlet calls check_unchanged(), which only cares about
> modifciation time.
> 
> I have a patch that makes it so that none of the tools do the
> check_unchanged logic themselves and instead rely on the
> library code to handle it for them.  This makes the
> implementation uniform across all tools, and allows tools to
> opt-in to trustExitCode=true.
> 
> This means that all of the builtin tools will default to
> trustExitCode=false, and they can opt-in by setting the
> configuration variable.
> 
> For tkdiff and kdiff3, this is a subtle change in behavior, but
> not one that should be problematic, and the upside is that we'll
> have consistency across all tools.
> 
> In this scenario specifically, what happens is that the
> scriptlet is calling check_unchanged(), which checks the
> modification time of the file, and if the file is new then it
> assumes that the merge succeeded.  check_unchanged() is clearing
> the exit code.
> 
> Try the patch below.  I tested it with vimdiff and it seems to
> provide the desired behavior:
> - the modificaiton time behavior is the default
> - setting mergetool.vimdiff.trustExitCode = true will make it
>   honor vim's exit code via :cq
> 
> One possible idea that could avoid the subtle tkdiff/kdiff3
> change in behavior would be to allow the scriptlets to advertise
> their preference for the default trustExitCode setting.  These
> tools could say, "default to true", and the rest can assume
> false.
> 
> If others feel that this is worth the extra machinery, and the
> mental burden of tools having different defaults, then that
> could be implemented as a follow-up patch.  IMO I'd be okay with
> not needing it and only adding it if someone notices, but if
> others feel otherwise we can do it sooner rather than later.
> 
> Thoughts?

For the curious, here is what that patch might look like.
This allows scriptlets to redefine trust_exit_code() so that
they can advertise that they prefer default=true.

The main benefit of this is that we're preserving the original
behavior before these patches.  I'll let this sit out here for
comments for a few days to see what others think.

--- >8 ---
Date: Sun, 27 Nov 2016 18:08:08 -0800
Subject: [PATCH] mergetool: restore trustExitCode behavior for builtins tools

deltawalker, diffmerge, emerge, kdiff3, kompare, and tkdiff originally
provided behavior that matched trustExitCode=true.

The default for all tools is trustExitCode=false, which conflicts with
these tools' defaults.  Allow tools to advertise their own default value
for trustExitCode so that users do not need to opt-in to the original
behavior.

While this makes the default inconsistent between tools, it can still be
overridden, and it makes it consistent with the current Git behavior.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-mergetool--lib.sh  | 15 +--
 mergetools/deltawalker |  6 ++
 mergetools/diffmerge   |  6 ++
 mergetools/emerge  |  6 ++
 mergetools/kdiff3  |  6 ++
 mergetools/kompare |  6 ++
 mergetools/tkdiff  |  6 ++
 7 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 3d8a873ab..be079723a 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -120,6 +120,12 @@ setup_user_tool () {
merge_tool_cmd=$(get_merge_tool_cmd "$tool")
test -n "$merge_tool_cmd" || return 1
 
+   trust_exit_code () {
+   # user-defined tools default to trustExitCode = false
+   git config --bool "mergetool.$1.trustExitCode" ||
+   echo false
+   }
+
diff_cmd () {
( eval $merge_tool_cmd )
}
@@ -153,6 +159,12 @@ setup_tool () {
echo "$1"
}
 
+   trust_exit_code () {
+   # built-in tools default to trustExitCode = false
+   git config --bool "mergetool.$1.tr

Re: trustExitCode doesn't apply to vimdiff mergetool

[David Aguilar]

On Sun, Nov 27, 2016 at 11:55:59AM -0500, Jeff King wrote:
> On Sun, Nov 27, 2016 at 08:46:40AM -0500, Dun Peal wrote:
> 
> > Ignoring a non-zero exit code from the merge tool, and assuming a
> > successful merge in that case, seems like the wrong default behavior
> > to me.
> 
> Yeah, I'm inclined to agree. But like I said, I'm not too familiar with
> this area, so maybe there are subtle things I'm missing.

I think this may have been an oversight in how the
trust-exit-code feature is implemented across builtins.

Right now, specific builtin tools could in theory opt-in to the
feature, but I think it should be handled in a central place.
For vimdiff, the exit code is not considered because the
scriptlet calls check_unchanged(), which only cares about
modifciation time.

I have a patch that makes it so that none of the tools do the
check_unchanged logic themselves and instead rely on the
library code to handle it for them.  This makes the
implementation uniform across all tools, and allows tools to
opt-in to trustExitCode=true.

This means that all of the builtin tools will default to
trustExitCode=false, and they can opt-in by setting the
configuration variable.

For tkdiff and kdiff3, this is a subtle change in behavior, but
not one that should be problematic, and the upside is that we'll
have consistency across all tools.

In this scenario specifically, what happens is that the
scriptlet is calling check_unchanged(), which checks the
modification time of the file, and if the file is new then it
assumes that the merge succeeded.  check_unchanged() is clearing
the exit code.

Try the patch below.  I tested it with vimdiff and it seems to
provide the desired behavior:
- the modificaiton time behavior is the default
- setting mergetool.vimdiff.trustExitCode = true will make it
  honor vim's exit code via :cq

One possible idea that could avoid the subtle tkdiff/kdiff3
change in behavior would be to allow the scriptlets to advertise
their preference for the default trustExitCode setting.  These
tools could say, "default to true", and the rest can assume
false.

If others feel that this is worth the extra machinery, and the
mental burden of tools having different defaults, then that
could be implemented as a follow-up patch.  IMO I'd be okay with
not needing it and only adding it if someone notices, but if
others feel otherwise we can do it sooner rather than later.

Thoughts?

--- 8< ---
Date: Sun, 27 Nov 2016 17:26:55 -0800
Subject: [PATCH] mergetool: honor mergetool..trustExitCode for all tools

The built-in mergetools originally required that each tool scriptlet
opt-in to the trustExitCode behavior, based on whether or not the tool
called check_unchanged() itself.

Refactor the functions so that run_merge_cmd() (rather than merge_cmd())
takes care of calling check_unchanged() so that all tools handle
the trustExitCode behavior uniformly.

Remove the check_unchanged() calls from the scriptlets.
A subtle benefit of this change is that the responsibility of
merge_cmd() has been narrowed to running the command only,
rather than also needing to deal with the backup file and
checking for changes.

Reported-by: Dun Peal <dunpea...@gmail.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-mergetool--lib.sh| 24 ++--
 mergetools/araxis|  2 --
 mergetools/bc|  2 --
 mergetools/codecompare   |  2 --
 mergetools/diffuse   |  2 --
 mergetools/ecmerge   |  2 --
 mergetools/examdiff  |  2 --
 mergetools/meld  |  3 +--
 mergetools/opendiff  |  2 --
 mergetools/p4merge   |  2 --
 mergetools/tortoisemerge |  2 --
 mergetools/vimdiff   |  2 --
 mergetools/winmerge  |  2 --
 mergetools/xxdiff|  2 --
 14 files changed, 15 insertions(+), 36 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9abd00be2..3d8a873ab 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -125,16 +125,7 @@ setup_user_tool () {
}
 
merge_cmd () {
-   trust_exit_code=$(git config --bool \
-   "mergetool.$1.trustExitCode" || echo false)
-   if test "$trust_exit_code" = "false"
-   then
-   touch "$BACKUP"
-   ( eval $merge_tool_cmd )
-   check_unchanged
-   else
-   ( eval $merge_tool_cmd )
-   fi
+   ( eval $merge_tool_cmd )
}
 }
 
@@ -225,7 +216,20 @@ run_diff_cmd () {
 
 # Run a either a configured or built-in merge tool
 run_merge_cmd () {
+   touch "$BACKUP"
+
merge_cmd "$1"
+   status=$?
+
+   trust_exit_code=$(git config --bool \
+   "mergetool.$1.trustExitCode" || echo false)
+   if test "$trust_exit_code" = "false"
+   then
+  

Re: [PATCH 1/2] difftool: add the builtin

[David Aguilar]

On Tue, Nov 22, 2016 at 06:01:23PM +0100, Johannes Schindelin wrote:
> This adds a builtin difftool that represents a conversion of the current
> Perl script version of the difftool.
> 
> The motivation is that Perl scripts are not at all native on Windows,
> and that `git difftool` therefore is pretty slow on that platform, when
> there is no good reason for it to be slow.
> 
> In addition, Perl does not really have access to Git's internals. That
> means that any script will always have to jump through unnecessary
> hoops.

Nice!

> The current version of the builtin difftool does not, however, make full
> use of the internals but instead chooses to spawn a couple of Git
> processes, still, to make for an easier conversion. There remains a lot
> of room for improvement, left for a later date.
> 
> Note: the original difftool is still called by `git difftool`. To get the
> new, experimental version, call `git builtin-difftool`. The reason: this
> new, experimental, builtin difftool will be shipped as part of Git for
> Windows v2.11.0, to allow for easier large-scale testing.

I like this plan.  I was going to ask for an environment
variable (to preset in git-cola) but since Git for Windows is
handling it then everyone benefits.

> diff --git a/builtin/builtin-difftool.c b/builtin/builtin-difftool.c
> new file mode 100644
> index 000..9feefcd
> --- /dev/null
> +++ b/builtin/builtin-difftool.c
> @@ -0,0 +1,680 @@
> +/*
> + * "git difftool" builtin command
> + *
> + * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
> + * git-difftool--helper script.
> + *
> + * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
> + * The GIT_DIFF* variables are exported for use by git-difftool--helper.
> + *
> + * Any arguments that are unknown to this script are forwarded to 'git diff'.
> + *
> + * Copyright (C) 2016 Johannes Schindelin
> + */
> +#include "cache.h"
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "run-command.h"
> +#include "argv-array.h"
> +#include "strbuf.h"
> +#include "lockfile.h"
> +
> +static char *diff_gui_tool;
> +static int trust_exit_code;
> +
> +static const char * const builtin_difftool_usage[] = {
> + N_("git add [] [--] ..."),
> + NULL
> +};

The usage should probably say "difftool" (or "builtin-difftool").

> [...]
> +static void changed_files(struct hashmap *result, const char *index_path,
> +   const char *workdir)
> +{
> +[...]
> +}
> +
> +#include "dir.h"

Can this mid-file #include go to the top of the file?

> +static int run_dir_diff(const char *extcmd, int symlinks,
> + int argc, const char **argv)
> +{
> + char tmpdir[PATH_MAX];
> + struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
> + struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
> + struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
> + struct strbuf wtdir = STRBUF_INIT;
> + size_t ldir_len, rdir_len, wtdir_len;
> + struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
> + const char *workdir, *tmp;
> + int ret = 0, i;
> + FILE *fp;
> + struct hashmap working_tree_dups, submodules, symlinks2;
> + struct hashmap_iter iter;
> + struct pair_entry *entry;
> + enum object_type type;
> + unsigned long size;
> + struct index_state wtindex;
> + struct checkout lstate, rstate;
> + int rc, flags = RUN_GIT_CMD, err = 0;
> + struct child_process child = CHILD_PROCESS_INIT;
> + const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
> + struct hashmap wt_modified, tmp_modified;
> + int indices_loaded = 0;
> +
> + setup_work_tree();
> + workdir = get_git_work_tree();
> +
> + /* Setup temp directories */
> + tmp = getenv("TMPDIR");
> + sprintf(tmpdir, "%s/git-difftool.XX", tmp ? tmp : "/tmp");

Maybe snprintf instead?

getenv() won't return anything longer than PATH_MAX for most
users, but users are weird.

> + if (!mkdtemp(tmpdir))
> + return error("could not create temporary directory");

Mention the tmpdir here?

> + strbuf_addf(, "%s/left/", tmpdir);
> + strbuf_addf(, "%s/right/", tmpdir);
> + strbuf_addstr(, workdir);
> + if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
> + strbuf_addch(, '/');
> + mkdir(ldir.buf, 0777);
> + mkdir(rdir.buf, 0777);

Seeing the perl mkpath() default 0777 spelled out this way
makes me wonder whether 0700 would be safer.

The mkdtemp() above is already using 0700 so it's ok, but it
might be worth making it consistent (later, perhaps).

> [...]
> +int cmd_builtin_difftool(int argc, const char ** argv, const char * prefix)
> +{
> + int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
> + tool_help = 0;
> + static char *difftool_cmd = NULL, *extcmd = NULL;
> +
> + struct option builtin_difftool_options[] = {
> + OPT_BOOL('g', "gui", _gui_tool,
> +  

Re: [PATCH 1/2] Feature Request: user defined suffix for temp files created by git-mergetool

[David Aguilar]

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

Re: [PATCH v2 2/2] Feature Request: user defined suffix for temp files created by git-mergetool

[David Aguilar]

On Wed, Oct 12, 2016 at 10:59:46AM -0700, Junio C Hamano wrote:
> Josef Ridky  writes:
> 
> > This is update of the second 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 
> > ---
> 
> David, what do you think?
> 
> I don't think you were ever CC'ed on any of the messages in
> this thread, and I don't think you've commented on the topic.  The
> thread begins here:
> 
>   
> https://public-inbox.org/git/1329039097.128066.1475476591437.javamail.zim...@redhat.com/
> 
> 
> In any case, I suggest update to the log message to something like
> this:
> 
> Subject: mergetool: allow custom naming for temporary files
> 
> A front-end program that is spawned by "git mergetool" is given
> three temporary files (e.g. it may get "x_LOCAL.txt",
> "x_REMOTE.txt", and "x_BASE.txt" while merging "x.txt").  
> 
> Custom wrappers to "git mergetool" benefits if they are allowed
> to rename these hardcoded suffixes to match the workflow they
> implement.  For example, they may be used to compare and merge
> two versions that is available locally, and OLD/NEW may be more
> appropriate than LOCAL/REMOTE in such a context.
> 
> primarily because "the second variant" is meaningless thing to say
> in our long term history, when the first variant never was recorded
> there.  Josef may want to elaborate more on the latter paragraph.


I do see why this can be helpful but what makes me reluctant is,

> > +'git mergetool' [--tool=] [-y | --[no-]prompt] [--local=] 
> > [--remote=] [--backup=] [--base=] [...]

We're parking on 4 new flags that are really all about changing
one small aspect of the program.

I don't typically like going overboard with environment
variables but for this use case they seem to be a good fit.

It's already been expressed that the intention is for these to
be supplied by some other tool, and environment variables have
the nice property that you can update all your tools without
needing to touch anything except the environment.

Another nice thing is that the the user can choose to set it
globally, or to set it local to specific tools.

Another reason why I think it's a good fit is,

> +# Can be changed by user
> +LOCAL_NAME='LOCAL'
> +BASE_NAME='BASE'
> +BACKUP_NAME='BACKUP'
> +REMOTE_NAME='REMOTE'

These lines would become simply,

LOCAL_NAME=${GIT_MERGETOOL_LOCAL_NAME:-LOCAL}
BASE_NAME=${GIT_MERGETOOL_BASE_NAME:-BASE}
BACKUP_NAME=${GIT_MERGETOOL_BACKUP_NAME:-BACKUP}
REMOTE_NAME=${GIT_MERGETOOL_REMOTE_NAME:-REMOTE}

and we wouldn't need any other change besides this part:

> - BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
> - LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
> - REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
> - BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
> + BACKUP="$MERGETOOL_TMPDIR/${BASE}_${BACKUP_NAME}_$$$ext"
> + LOCAL="$MERGETOOL_TMPDIR/${BASE}_${LOCAL_NAME}_$$$ext"
> + REMOTE="$MERGETOOL_TMPDIR/${BASE}_${REMOTE_NAME}_$$$ext"
> + BASE="$MERGETOOL_TMPDIR/${BASE}_${BASE_NAME}_$$$ext"


Then, just update the documentation to mention the environment
variables instead of the flags and we're all set.

We also won't need this part if we go down that route:

> +# sanity check after parsing command line
> +case "" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/remote/base/backup to empty."
> + ;;
> +esac
> +
> +case "$LOCAL_NAME" in
> +"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --remote/base/backup to same as --local."
> + ;;
> +esac
> +
> +case "$REMOTE_NAME" in
> +"$LOCAL_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/base/backup to same as --remote."
> + ;;
> +esac
> +
> +case "$BASE_NAME" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/remote/backup to same as --base."
> + ;;
> +esac
> +
> +case "$BACKUP_NAME" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME")
> + die "You cannot set any of --local/remote/base to same as --backup."
> + ;;
> +esac


What do you think?
-- 
David

Re: [PATCH v4 4/4] mergetool: honor -O

[David Aguilar]

On Mon, Oct 10, 2016 at 11:28:35AM -0700, Junio C Hamano wrote:
> David Aguilar <dav...@gmail.com> writes:
> 
> > Teach mergetool to pass "-O" down to `git diff` when
> > specified on the command-line.
> >
> > Helped-by: Johannes Sixt <j...@kdbg.org>
> > Signed-off-by: David Aguilar <dav...@gmail.com>
> > ---
> > Since v3:
> >
> > I missed one last piped invocation of "git mergetool" in the tests,
> > which has been fixed.
> 
> I only see 4/4 in v4; am I correct to assume that 1-3/4 of v4 are
> the same as their counterparts in v3?

Yes, 1-3 are unchanged since v3.
Thanks for checking,
-- 
David

[PATCH v4 4/4] mergetool: honor -O

[David Aguilar]

Teach mergetool to pass "-O" down to `git diff` when
specified on the command-line.

Helped-by: Johannes Sixt <j...@kdbg.org>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
Since v3:

I missed one last piped invocation of "git mergetool" in the tests,
which has been fixed.

 Documentation/git-mergetool.txt | 10 ++
 git-mergetool.sh|  9 +++--
 t/t7610-mergetool.sh| 27 +++
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c4c1a9b..3622d66 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,10 +79,12 @@ 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.
 
-DIFF ORDER FILES
-
-`git mergetool` honors the `diff.orderFile` configuration variable
-used by `git diff`.  See linkgit:git-config[1] for more details.
+-O::
+   Process files in the order specified in the
+   , which has one shell glob pattern per line.
+   This overrides the `diff.orderFile` configuration variable
+   (see linkgit:git-config[1]).  To cancel `diff.orderFile`,
+   use `-O/dev/null`.
 
 TEMPORARY FILES
 ---
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 65696d8..e52b4e4 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,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] [-O] 
[file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -390,6 +390,7 @@ print_noop_and_exit () {
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
+   orderfile=
 
while test $# != 0
do
@@ -419,6 +420,9 @@ main () {
--prompt)
prompt=true
;;
+   -O*)
+   orderfile="$1"
+   ;;
--)
shift
break
@@ -460,7 +464,8 @@ main () {
fi
 
files=$(git -c core.quotePath=false \
-   diff --name-only --diff-filter=U -- "$@")
+   diff --name-only --diff-filter=U \
+   ${orderfile:+"$orderfile"} -- "$@")
 
cd_to_toplevel
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 38c1e4d..6d9f215 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -638,5 +638,32 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
test_cmp expect actual &&
git reset --hard >/dev/null
 '
+test_expect_success 'mergetool -Oorder-file is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   a
+   b
+   EOF
+   git mergetool -O/dev/null --no-prompt --tool myecho >output &&
+   git grep --no-index -h -A2 Merging: output >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1 &&
+
+   git config --unset diff.orderFile &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool -Oorder-file --no-prompt --tool myecho >output &&
+   git grep --no-index -h -A2 Merging: output >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1
+'
 
 test_done
-- 
2.10.1.386.g8ee99a0

[PATCH v3 2/4] mergetool: move main program flow into a main() function

[David Aguilar]

Make it easier to follow the program's flow by isolating all
logic into functions.  Isolate the main execution code path into
a single unit instead of having prompt_after_failed_merge()
interrupt it partyway through.

The use of a main() function is borrowing a convention from C,
Python, Perl, and many other languages.  This helps readers more
familiar with other languages understand the purpose of each
function when diving into the codebase with fresh eyes.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
Unchanged since v2; included for completeness.

 git-mergetool.sh | 180 ---
 1 file changed, 93 insertions(+), 87 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 300ce7f..b2cd0a4 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -366,51 +366,6 @@ merge_file () {
return 0
 }
 
-prompt=$(git config --bool mergetool.prompt)
-guessed_merge_tool=false
-
-while test $# != 0
-do
-   case "$1" in
-   --tool-help=*)
-   TOOL_MODE=${1#--tool-help=}
-   show_tool_help
-   ;;
-   --tool-help)
-   show_tool_help
-   ;;
-   -t|--tool*)
-   case "$#,$1" in
-   *,*=*)
-   merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
-   ;;
-   1,*)
-   usage ;;
-   *)
-   merge_tool="$2"
-   shift ;;
-   esac
-   ;;
-   -y|--no-prompt)
-   prompt=false
-   ;;
-   --prompt)
-   prompt=true
-   ;;
-   --)
-   shift
-   break
-   ;;
-   -*)
-   usage
-   ;;
-   *)
-   break
-   ;;
-   esac
-   shift
-done
-
 prompt_after_failed_merge () {
while true
do
@@ -427,57 +382,108 @@ prompt_after_failed_merge () {
done
 }
 
-git_dir_init
-require_work_tree
+main () {
+   prompt=$(git config --bool mergetool.prompt)
+   guessed_merge_tool=false
+
+   while test $# != 0
+   do
+   case "$1" in
+   --tool-help=*)
+   TOOL_MODE=${1#--tool-help=}
+   show_tool_help
+   ;;
+   --tool-help)
+   show_tool_help
+   ;;
+   -t|--tool*)
+   case "$#,$1" in
+   *,*=*)
+   merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
+   ;;
+   1,*)
+   usage ;;
+   *)
+   merge_tool="$2"
+   shift ;;
+   esac
+   ;;
+   -y|--no-prompt)
+   prompt=false
+   ;;
+   --prompt)
+   prompt=true
+   ;;
+   --)
+   shift
+   break
+   ;;
+   -*)
+   usage
+   ;;
+   *)
+   break
+   ;;
+   esac
+   shift
+   done
+
+   git_dir_init
+   require_work_tree
 
-if test -z "$merge_tool"
-then
-   # Check if a merge tool has been configured
-   merge_tool=$(get_configured_merge_tool)
-   # Try to guess an appropriate merge tool if no tool has been set.
if test -z "$merge_tool"
then
-   merge_tool=$(guess_merge_tool) || exit
-   guessed_merge_tool=true
+   # Check if a merge tool has been configured
+   merge_tool=$(get_configured_merge_tool)
+   # Try to guess an appropriate merge tool if no tool has been 
set.
+   if test -z "$merge_tool"
+   then
+   merge_tool=$(guess_merge_tool) || exit
+   guessed_merge_tool=true
+   fi
fi
-fi
-merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
-merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo 
false)"
-
-files=
+   merge_keep_backup="$(git config --bool mergetool.keepBackup || echo 
true)"
+   merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries 
|| echo false)"
 
-if test $# -eq 0
-then
-   cd_to_toplevel
+   files=
 
-   if test -e "$GIT_DIR/MERGE_RR"
+   if test $# -eq 0
then
-   files=$(git rerere remaining)
+   cd_to_toplevel
+
+   if test 

[PATCH v3 3/4] mergetool: honor diff.orderFile

[David Aguilar]

Teach mergetool to get the list of files to edit via `diff` so that we
gain support for diff.orderFile.

Suggested-by: Luis Gutierrez <luisg...@gmail.com>
Helped-by: Johannes Sixt <j...@kdbg.org>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
Changes since v2:

The tests no longer rely on "grep -A" and instead use "git grep"
for portability.  The mergetool output in the test is redirected
to a file so that we can catch its exit code.

 Documentation/git-mergetool.txt |  5 +
 git-mergetool.sh| 30 +++---
 t/t7610-mergetool.sh| 33 +
 3 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e846c2e..c4c1a9b 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -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.
 
+DIFF ORDER FILES
+
+`git mergetool` honors the `diff.orderFile` configuration variable
+used by `git diff`.  See linkgit:git-config[1] for more details.
+
 TEMPORARY FILES
 ---
 `git mergetool` creates `*.orig` backup files while resolving merges.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index b2cd0a4..65696d8 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -382,6 +382,11 @@ prompt_after_failed_merge () {
done
 }
 
+print_noop_and_exit () {
+   echo "No files need merging"
+   exit 0
+}
+
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
@@ -445,28 +450,23 @@ main () {
merge_keep_backup="$(git config --bool mergetool.keepBackup || echo 
true)"
merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries 
|| echo false)"
 
-   files=
-
-   if test $# -eq 0
+   if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
then
-   cd_to_toplevel
-
-   if test -e "$GIT_DIR/MERGE_RR"
+   set -- $(git rerere remaining)
+   if test $# -eq 0
then
-   files=$(git rerere remaining)
-   else
-   files=$(git ls-files -u |
-   sed -e 's/^[^   ]*  //' | sort -u)
+   print_noop_and_exit
fi
-   else
-   files=$(git ls-files -u -- "$@" |
-   sed -e 's/^[^   ]*  //' | sort -u)
fi
 
+   files=$(git -c core.quotePath=false \
+   diff --name-only --diff-filter=U -- "$@")
+
+   cd_to_toplevel
+
if test -z "$files"
then
-   echo "No files need merging"
-   exit 0
+   print_noop_and_exit
fi
 
printf "Merging:\n"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 7217f39..38c1e4d 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -606,4 +606,37 @@ test_expect_success MKTEMP 'temporary filenames are used 
with mergetool.writeToT
git reset --hard master >/dev/null 2>&1
 '
 
+test_expect_success 'diff.orderFile configuration is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   echo b >order-file &&
+   echo a >>order-file &&
+   git checkout -b order-file-start master &&
+   echo start >a &&
+   echo start >b &&
+   git add a b &&
+   git commit -m start &&
+   git checkout -b order-file-side1 order-file-start &&
+   echo side1 >a &&
+   echo side1 >b &&
+   git add a b &&
+   git commit -m side1 &&
+   git checkout -b order-file-side2 order-file-start &&
+   echo side2 >a &&
+   echo side2 >b &&
+   git add a b &&
+   git commit -m side2 &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool --no-prompt --tool myecho >output &&
+   git grep --no-index -h -A2 Merging: output >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null
+'
+
 test_done
-- 
2.10.1.386.g8ee99a0

[PATCH v3 4/4] mergetool: honor -O

[David Aguilar]

Teach mergetool to pass "-O" down to `git diff` when
specified on the command-line.

Helped-by: Johannes Sixt <j...@kdbg.org>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
Changes since v2:

The tests no longer rely on "grep -A" and instead use "git grep"
for portability.  The mergetool output in the test is redirected
to a file so that we can catch its exit code.

The $orderfile variable is now passed using ${xxx:+"$xxx"}
to avoid conditional logic.

 Documentation/git-mergetool.txt | 10 ++
 git-mergetool.sh|  9 +++--
 t/t7610-mergetool.sh| 27 +++
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c4c1a9b..3622d66 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,10 +79,12 @@ 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.
 
-DIFF ORDER FILES
-
-`git mergetool` honors the `diff.orderFile` configuration variable
-used by `git diff`.  See linkgit:git-config[1] for more details.
+-O::
+   Process files in the order specified in the
+   , which has one shell glob pattern per line.
+   This overrides the `diff.orderFile` configuration variable
+   (see linkgit:git-config[1]).  To cancel `diff.orderFile`,
+   use `-O/dev/null`.
 
 TEMPORARY FILES
 ---
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 65696d8..e52b4e4 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,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] [-O] 
[file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -390,6 +390,7 @@ print_noop_and_exit () {
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
+   orderfile=
 
while test $# != 0
do
@@ -419,6 +420,9 @@ main () {
--prompt)
prompt=true
;;
+   -O*)
+   orderfile="$1"
+   ;;
--)
shift
break
@@ -460,7 +464,8 @@ main () {
fi
 
files=$(git -c core.quotePath=false \
-   diff --name-only --diff-filter=U -- "$@")
+   diff --name-only --diff-filter=U \
+   ${orderfile:+"$orderfile"} -- "$@")
 
cd_to_toplevel
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 38c1e4d..5cfad76 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -638,5 +638,32 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
test_cmp expect actual &&
git reset --hard >/dev/null
 '
+test_expect_success 'mergetool -Oorder-file is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   a
+   b
+   EOF
+   git mergetool -O/dev/null --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1 &&
+
+   git config --unset diff.orderFile &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool -Oorder-file --no-prompt --tool myecho >output &&
+   git grep --no-index -h -A2 Merging: output >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1
+'
 
 test_done
-- 
2.10.1.386.g8ee99a0

[PATCH v3 1/4] mergetool: add copyright

[David Aguilar]

Signed-off-by: David Aguilar <dav...@gmail.com>
---
Unchanged since v1; included for completeness.

 git-mergetool.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index bf86270..300ce7f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -3,6 +3,7 @@
 # This program resolves merge conflicts in git
 #
 # Copyright (c) 2006 Theodore Y. Ts'o
+# Copyright (c) 2009-2016 David Aguilar
 #
 # This file is licensed under the GPL v2, or a later version
 # at the discretion of Junio C Hamano.
-- 
2.10.1.386.g8ee99a0

[PATCH v2 2/4] mergetool: move main program flow into a main() function

[David Aguilar]

Make it easier to follow the program's flow by isolating all
logic into functions.  Isolate the main execution code path into
a single unit instead of having prompt_after_failed_merge()
interrupt it partyway through.

The use of a main() function is borrowing a convention from C,
Python, Perl, and many other languages.  This helps readers more
familiar with other languages understand the purpose of each
function when diving into the codebase with fresh eyes.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
As suggested by Hannes, v2 provides a better commit message.

This is a stylistic choice, but since I was about to start hacking on it,
I couldn't help but notice the organic sprawl that could use some tidying
before adding new features.

 git-mergetool.sh | 180 ---
 1 file changed, 93 insertions(+), 87 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 300ce7f..b2cd0a4 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -366,51 +366,6 @@ merge_file () {
return 0
 }
 
-prompt=$(git config --bool mergetool.prompt)
-guessed_merge_tool=false
-
-while test $# != 0
-do
-   case "$1" in
-   --tool-help=*)
-   TOOL_MODE=${1#--tool-help=}
-   show_tool_help
-   ;;
-   --tool-help)
-   show_tool_help
-   ;;
-   -t|--tool*)
-   case "$#,$1" in
-   *,*=*)
-   merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
-   ;;
-   1,*)
-   usage ;;
-   *)
-   merge_tool="$2"
-   shift ;;
-   esac
-   ;;
-   -y|--no-prompt)
-   prompt=false
-   ;;
-   --prompt)
-   prompt=true
-   ;;
-   --)
-   shift
-   break
-   ;;
-   -*)
-   usage
-   ;;
-   *)
-   break
-   ;;
-   esac
-   shift
-done
-
 prompt_after_failed_merge () {
while true
do
@@ -427,57 +382,108 @@ prompt_after_failed_merge () {
done
 }
 
-git_dir_init
-require_work_tree
+main () {
+   prompt=$(git config --bool mergetool.prompt)
+   guessed_merge_tool=false
+
+   while test $# != 0
+   do
+   case "$1" in
+   --tool-help=*)
+   TOOL_MODE=${1#--tool-help=}
+   show_tool_help
+   ;;
+   --tool-help)
+   show_tool_help
+   ;;
+   -t|--tool*)
+   case "$#,$1" in
+   *,*=*)
+   merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
+   ;;
+   1,*)
+   usage ;;
+   *)
+   merge_tool="$2"
+   shift ;;
+   esac
+   ;;
+   -y|--no-prompt)
+   prompt=false
+   ;;
+   --prompt)
+   prompt=true
+   ;;
+   --)
+   shift
+   break
+   ;;
+   -*)
+   usage
+   ;;
+   *)
+   break
+   ;;
+   esac
+   shift
+   done
+
+   git_dir_init
+   require_work_tree
 
-if test -z "$merge_tool"
-then
-   # Check if a merge tool has been configured
-   merge_tool=$(get_configured_merge_tool)
-   # Try to guess an appropriate merge tool if no tool has been set.
if test -z "$merge_tool"
then
-   merge_tool=$(guess_merge_tool) || exit
-   guessed_merge_tool=true
+   # Check if a merge tool has been configured
+   merge_tool=$(get_configured_merge_tool)
+   # Try to guess an appropriate merge tool if no tool has been 
set.
+   if test -z "$merge_tool"
+   then
+   merge_tool=$(guess_merge_tool) || exit
+   guessed_merge_tool=true
+   fi
fi
-fi
-merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
-merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo 
false)"
-
-files=
+   merge_keep_backup="$(git config --bool mergetool.keepBackup || echo 
true)"
+   merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries 
|| echo false)"
 
-if test $# -eq 0
-then
-   cd_to_toplevel
+   files=

[PATCH v2 4/4] mergetool: honor -O

[David Aguilar]

Teach mergetool to pass "-O" down to `git diff` when
specified on the command-line.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
This is a replacement patch for 4/4 from the original series.

The changes are stylistic -- the "order_file" variable name and
"-O" in the usage were changed to "orderfile" and
"-O" for consistency with the documentation.

 Documentation/git-mergetool.txt | 10 ++
 git-mergetool.sh| 14 --
 t/t7610-mergetool.sh| 27 +++
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c4c1a9b..3622d66 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,10 +79,12 @@ 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.
 
-DIFF ORDER FILES
-
-`git mergetool` honors the `diff.orderFile` configuration variable
-used by `git diff`.  See linkgit:git-config[1] for more details.
+-O::
+   Process files in the order specified in the
+   , which has one shell glob pattern per line.
+   This overrides the `diff.orderFile` configuration variable
+   (see linkgit:git-config[1]).  To cancel `diff.orderFile`,
+   use `-O/dev/null`.
 
 TEMPORARY FILES
 ---
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 65696d8..1a0fe7a 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,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] [-O] 
[file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -390,6 +390,7 @@ print_noop_and_exit () {
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
+   orderfile=
 
while test $# != 0
do
@@ -419,6 +420,9 @@ main () {
--prompt)
prompt=true
;;
+   -O*)
+   orderfile="$1"
+   ;;
--)
shift
break
@@ -459,8 +463,14 @@ main () {
fi
fi
 
+   if test -n "$orderfile"
+   then
+   set -- "$orderfile" -- "$@"
+   else
+   set -- -- "$@"
+   fi
files=$(git -c core.quotePath=false \
-   diff --name-only --diff-filter=U -- "$@")
+   diff --name-only --diff-filter=U "$@")
 
cd_to_toplevel
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index fb2aeef..0f9869a 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -637,6 +637,33 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
test_cmp expect actual &&
git reset --hard >/dev/null
 '
+
+test_expect_success 'mergetool -Oorder-file is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   a
+   b
+   EOF
+   git mergetool -O/dev/null --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1 &&
+
+   git config --unset diff.orderFile &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool -Oorder-file --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1
 '
 
 test_done
-- 
2.10.1.386.g42aabb0

[PATCH 3/4] mergetool: honor diff.orderFile

[David Aguilar]

Teach mergetool to get the list of files to edit via `diff` so that we
gain support for diff.orderFile.

Suggested-by: Luis Gutierrez <luisg...@gmail.com>
Helped-by: Johannes Sixt <j...@kdbg.org>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
 Documentation/git-mergetool.txt |  5 +
 git-mergetool.sh| 30 +++---
 t/t7610-mergetool.sh| 33 +
 3 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e846c2e..c4c1a9b 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -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.
 
+DIFF ORDER FILES
+
+`git mergetool` honors the `diff.orderFile` configuration variable
+used by `git diff`.  See linkgit:git-config[1] for more details.
+
 TEMPORARY FILES
 ---
 `git mergetool` creates `*.orig` backup files while resolving merges.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index b2cd0a4..65696d8 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -382,6 +382,11 @@ prompt_after_failed_merge () {
done
 }
 
+print_noop_and_exit () {
+   echo "No files need merging"
+   exit 0
+}
+
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
@@ -445,28 +450,23 @@ main () {
merge_keep_backup="$(git config --bool mergetool.keepBackup || echo 
true)"
merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries 
|| echo false)"
 
-   files=
-
-   if test $# -eq 0
+   if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
then
-   cd_to_toplevel
-
-   if test -e "$GIT_DIR/MERGE_RR"
+   set -- $(git rerere remaining)
+   if test $# -eq 0
then
-   files=$(git rerere remaining)
-   else
-   files=$(git ls-files -u |
-   sed -e 's/^[^   ]*  //' | sort -u)
+   print_noop_and_exit
fi
-   else
-   files=$(git ls-files -u -- "$@" |
-   sed -e 's/^[^   ]*  //' | sort -u)
fi
 
+   files=$(git -c core.quotePath=false \
+   diff --name-only --diff-filter=U -- "$@")
+
+   cd_to_toplevel
+
if test -z "$files"
then
-   echo "No files need merging"
-   exit 0
+   print_noop_and_exit
fi
 
printf "Merging:\n"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 7217f39..fb2aeef 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -606,4 +606,37 @@ test_expect_success MKTEMP 'temporary filenames are used 
with mergetool.writeToT
git reset --hard master >/dev/null 2>&1
 '
 
+test_expect_success 'diff.orderFile configuration is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   echo b >order-file &&
+   echo a >>order-file &&
+   git checkout -b order-file-start master &&
+   echo start >a &&
+   echo start >b &&
+   git add a b &&
+   git commit -m start &&
+   git checkout -b order-file-side1 order-file-start &&
+   echo side1 >a &&
+   echo side1 >b &&
+   git add a b &&
+   git commit -m side1 &&
+   git checkout -b order-file-side2 order-file-start &&
+   echo side2 >a &&
+   echo side2 >b &&
+   git add a b &&
+   git commit -m side2 &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool --no-prompt --tool myecho | grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null
+'
+'
+
 test_done
-- 
2.10.1.355.g33afd83.dirty

[PATCH 4/4] mergetool: honor -O

[David Aguilar]

Teach mergetool to pass "-O" down to `git diff` when
specified on the command-line.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 Documentation/git-mergetool.txt | 10 ++
 git-mergetool.sh| 14 --
 t/t7610-mergetool.sh| 27 +++
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c4c1a9b..3622d66 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,10 +79,12 @@ 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.
 
-DIFF ORDER FILES
-
-`git mergetool` honors the `diff.orderFile` configuration variable
-used by `git diff`.  See linkgit:git-config[1] for more details.
+-O::
+   Process files in the order specified in the
+   , which has one shell glob pattern per line.
+   This overrides the `diff.orderFile` configuration variable
+   (see linkgit:git-config[1]).  To cancel `diff.orderFile`,
+   use `-O/dev/null`.
 
 TEMPORARY FILES
 ---
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 65696d8..9dca58b 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,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] [-O] 
[file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -390,6 +390,7 @@ print_noop_and_exit () {
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
+   order_file=
 
while test $# != 0
do
@@ -419,6 +420,9 @@ main () {
--prompt)
prompt=true
;;
+   -O*)
+   order_file="$1"
+   ;;
--)
shift
break
@@ -459,8 +463,14 @@ main () {
fi
fi
 
+   if test -n "$order_file"
+   then
+   set -- "$order_file" -- "$@"
+   else
+   set -- -- "$@"
+   fi
files=$(git -c core.quotePath=false \
-   diff --name-only --diff-filter=U -- "$@")
+   diff --name-only --diff-filter=U "$@")
 
cd_to_toplevel
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index fb2aeef..0f9869a 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -637,6 +637,33 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
test_cmp expect actual &&
git reset --hard >/dev/null
 '
+
+test_expect_success 'mergetool -Oorder-file is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   a
+   b
+   EOF
+   git mergetool -O/dev/null --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1 &&
+
+   git config --unset diff.orderFile &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool -Oorder-file --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1
 '
 
 test_done
-- 
2.10.1.355.g33afd83.dirty

[PATCH 2/4] mergetool: move main program flow into a main() function

[David Aguilar]

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-mergetool.sh | 180 ---
 1 file changed, 93 insertions(+), 87 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 300ce7f..b2cd0a4 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -366,51 +366,6 @@ merge_file () {
return 0
 }
 
-prompt=$(git config --bool mergetool.prompt)
-guessed_merge_tool=false
-
-while test $# != 0
-do
-   case "$1" in
-   --tool-help=*)
-   TOOL_MODE=${1#--tool-help=}
-   show_tool_help
-   ;;
-   --tool-help)
-   show_tool_help
-   ;;
-   -t|--tool*)
-   case "$#,$1" in
-   *,*=*)
-   merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
-   ;;
-   1,*)
-   usage ;;
-   *)
-   merge_tool="$2"
-   shift ;;
-   esac
-   ;;
-   -y|--no-prompt)
-   prompt=false
-   ;;
-   --prompt)
-   prompt=true
-   ;;
-   --)
-   shift
-   break
-   ;;
-   -*)
-   usage
-   ;;
-   *)
-   break
-   ;;
-   esac
-   shift
-done
-
 prompt_after_failed_merge () {
while true
do
@@ -427,57 +382,108 @@ prompt_after_failed_merge () {
done
 }
 
-git_dir_init
-require_work_tree
+main () {
+   prompt=$(git config --bool mergetool.prompt)
+   guessed_merge_tool=false
+
+   while test $# != 0
+   do
+   case "$1" in
+   --tool-help=*)
+   TOOL_MODE=${1#--tool-help=}
+   show_tool_help
+   ;;
+   --tool-help)
+   show_tool_help
+   ;;
+   -t|--tool*)
+   case "$#,$1" in
+   *,*=*)
+   merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
+   ;;
+   1,*)
+   usage ;;
+   *)
+   merge_tool="$2"
+   shift ;;
+   esac
+   ;;
+   -y|--no-prompt)
+   prompt=false
+   ;;
+   --prompt)
+   prompt=true
+   ;;
+   --)
+   shift
+   break
+   ;;
+   -*)
+   usage
+   ;;
+   *)
+   break
+   ;;
+   esac
+   shift
+   done
+
+   git_dir_init
+   require_work_tree
 
-if test -z "$merge_tool"
-then
-   # Check if a merge tool has been configured
-   merge_tool=$(get_configured_merge_tool)
-   # Try to guess an appropriate merge tool if no tool has been set.
if test -z "$merge_tool"
then
-   merge_tool=$(guess_merge_tool) || exit
-   guessed_merge_tool=true
+   # Check if a merge tool has been configured
+   merge_tool=$(get_configured_merge_tool)
+   # Try to guess an appropriate merge tool if no tool has been 
set.
+   if test -z "$merge_tool"
+   then
+   merge_tool=$(guess_merge_tool) || exit
+   guessed_merge_tool=true
+   fi
fi
-fi
-merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
-merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo 
false)"
-
-files=
+   merge_keep_backup="$(git config --bool mergetool.keepBackup || echo 
true)"
+   merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries 
|| echo false)"
 
-if test $# -eq 0
-then
-   cd_to_toplevel
+   files=
 
-   if test -e "$GIT_DIR/MERGE_RR"
+   if test $# -eq 0
then
-   files=$(git rerere remaining)
+   cd_to_toplevel
+
+   if test -e "$GIT_DIR/MERGE_RR"
+   then
+   files=$(git rerere remaining)
+   else
+   files=$(git ls-files -u |
+   sed -e 's/^[^   ]*  //' | sort -u)
+   fi
else
-   files=$(git ls-files -u | sed -e 's/^[^ ]*  //' | sort -u)
+   files=$(git ls-files -u -- "$@" |
+   sed -e 's/^[^   ]*  //' | sort -u)
fi
-else
-   file

[PATCH 1/4] mergetool: add copyright

[David Aguilar]

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-mergetool.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index bf86270..300ce7f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -3,6 +3,7 @@
 # This program resolves merge conflicts in git
 #
 # Copyright (c) 2006 Theodore Y. Ts'o
+# Copyright (c) 2009-2016 David Aguilar
 #
 # This file is licensed under the GPL v2, or a later version
 # at the discretion of Junio C Hamano.
-- 
2.10.1.355.g33afd83.dirty

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[David Aguilar]

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

Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[David Aguilar]

On Wed, Aug 17, 2016 at 05:51:30PM -0700, Jacob Keller wrote:
> [snip]
> @@ -333,31 +326,23 @@ static void print_submodule_summary(struct rev_info 
> *rev, FILE *f,
>   strbuf_release();
>  }
>  
> -void show_submodule_summary(FILE *f, const char *path,
> +/* Helper function to display the submodule header line prior to the full
> + * summary output. If it can locate the submodule objects directory it will
> + * attempt to lookup both the left and right commits and put them into the
> + * left and right pointers.
> + */
> +static void show_submodule_header(FILE *f, const char *path,
>   const char *line_prefix,
>   unsigned char one[20], unsigned char two[20],
>   unsigned dirty_submodule, const char *meta,
> - const char *del, const char *add, const char *reset)
> + const char *reset,
> + struct commit **left, struct commit **right)


Note the pre-existing unsigned char[20]s in the signature above...


> [snip]
> @@ -381,14 +401,39 @@ void show_submodule_summary(FILE *f, const char *path,
>   strbuf_addf(, "%s:%s\n", fast_backward ? " (rewind)" : "", 
> reset);
>   fwrite(sb.buf, sb.len, 1, f);
>  
> - if (!message) /* only NULL if we succeeded in setting up the walk */
> - print_submodule_summary(, f, line_prefix, del, add, reset);
> + strbuf_release();
> +}
> +
> +void show_submodule_summary(FILE *f, const char *path,
> + const char *line_prefix,
> + unsigned char one[20], unsigned char two[20],
> + unsigned dirty_submodule, const char *meta,
> + const char *del, const char *add, const char *reset)


... and here too.

There's an ongoing effort to replace unsigned char sha1[20]
with struct object_id.  It might make sense to pass "one" and
"two" as "unsigned char*" instead of hard-coding the SHA1-length
here.  Alternatively, we could pass in the struct object_id*
instead.

The [20] in the show_submodule_header() function above is
pre-existing and not added by this patch, but that function's
signature is touched by this patch so it might make sense to
tidy that up while we're in here, possibly as a separate patch.
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-mergetool reverse file ordering

[David Aguilar]

Hi Luis and Hannes,

On Wed, Aug 17, 2016 at 09:35:56AM +0200, Johannes Sixt wrote:
> Am 17.08.2016 um 08:46 schrieb David Aguilar:
> > The only thing that using diff-files doesn't address is the
> > rerere support in mergetool where it processes the files in
> > the order specified by "git rerere remaining".  This is why I
> > initially thought we needed a generic sort-like command.
> 
> I see. This is actually an important code path. How about this code
> structure:
> 
> if test $# -eq 0
> then
>   cd_to_toplevel
> 
>   if test -e "$GIT_DIR/MERGE_RR"
>   then
>   set -- $(git rerere remaining)
>   fi
> fi
> files=$(git diff-files --name-only --diff-filter=U -- "$@")
> 

Beautiful.

> This does not require an enhancement of rerere-remaining and still captures
> all three cases that currently go through separate branches. (Throw in some
> version of --ignore-submodules= if necessary, but I guess it is not.)
> 
> We do have a problem if there are file names with spaces, but it is not a
> new problem.

Thanks for the heads-up about file names with spaces.  We set,

IFS='
'

in git-mergetool--lib.sh so file names with spaces should be ok.
Naturally, we won't be able to support paths with embedded
newlines, but that's not a new problem ;-)

We should probably also set core.quotePath=false when calling
diff-files so that git doesn't try to quote "unusual" paths,
e.g.

git -c core.quotePath=false diff-files ...

Lastly, for anyone that's curious, I was wondering why we were
passing "-u" to "sort", and why we won't need to use "uniq" in
the new version.

The reason is that "ls-files -u" lists the different index
stages separately, and thus it reports duplicate paths.

"diff-files" with "--diff-filter=U" does not do that, so that's
another benefit to be gained from this change.

I think we've touched all the bases now.

Luis, I hope that makes sense.  Let us know if any of this is
unclear.


ciao,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-mergetool reverse file ordering

[David Aguilar]

On Wed, Aug 17, 2016 at 08:10:46AM +0200, Johannes Sixt wrote:
> Am 17.08.2016 um 08:05 schrieb Johannes Sixt:
> > Am 17.08.2016 um 03:25 schrieb David Aguilar:
> > > Hmm, I do like the idea of reusing the diff orderFile, but a
> > > mechanism for sorting arbitrary inputs based on the orderFile
> > > isn't currently exposed in a way that mergetool could use it.
> > 
> > Instead of using 'git ls-files -u | sed ... | sort -u' you could use
> > 
> >   git diff-files --name-status -O... | sed -ne '/^U/s/^..//p'
> 
> Or even better:
> 
> git diff-files --name-only --diff-filter=U -O...

Nice!


> > > But, that sort is honestly kinda crude.  It can't implement the
> > > interesting case where we want bar.h to sort before bar.c but
> > > not foo.h.
> > > 

Note: I had a subtle typo above -- I meant to describe this desired
order:
bar.h
bar.c
foo.h
foo.c

which is not possible with an orderFile that has only:

*.h
*.c

But...

> > > If we did the sort option, we could have both.
> > 
> > I don't think that we need a new filter when the diff machinery is
> > capable of re-ordering files. We should enhance the diff machinery instead.
> > 

Reading the code more thoroughly, I fully agree.

Switching to a diff-files invocation lets us reuse the
diff.orderFile machinery and does not require exposing any
additional helpers.

While it would be *nice* if we had a way to sort foo.h before
foo.c and after bar.c, that's just a pie-in-the-sky idea.
Consistency is king.


The only thing that using diff-files doesn't address is the
rerere support in mergetool where it processes the files in
the order specified by "git rerere remaining".  This is why I
initially thought we needed a generic sort-like command.

That code path does not currently do any sorting (which may
explain why we didn't notice it if we were just looking for
"sort" invocations) but maybe that's an edge case that we don't
need to address initially (if at all).

Would it be okay for the rerere code path to not honor the
orderFile?  IMO if we documented the behavior then it would be
acceptable, and perhaps can be improved as a follow-up.

For the basic support the implementation becomes really
simple: switch to using diff-files instead of ls-files.

If we did want consistency in the "git rerere remaining" code
path, would it be acceptable to teach "rerere" about
"-O" so that we could drive it from mergetool?

I only suggest an option, and not config support, because I
would be hesitant to make "rerere remaining" unconditionally
honor the orderFile since that feature is diff-centric, while
"rerere remaining" is a different beast altogether.  Adding a
flag is a middle-ground that would allow mergetool to drive it
while not suddently changing rerere's behavior.


The patches could then be:

1. switch to diff-files, add tests, and document how
   diff.orderFile affects mergetool.

2. Teach mergetool about the "-O" flag so that it can
   override the configuration, and add tests.  It could be
   argued that this should be squashed into (1).

3. (optional) teach "rerere remaining" to honor the
   -O flag and teach mergetool to supply the option.

Sound good?


ciao,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-mergetool reverse file ordering

[David Aguilar]

On Mon, Aug 15, 2016 at 09:19:35PM +0100, Luis Gutierrez wrote:
> > Thoughts?  Would you be interested in helping work up a patch
> > for this idea?  At a minimum we should also write a test case in
> > t/t7610-mergetool.sh to verify that it works as advertised.
> 
> > Why not reuse the existing diff.orderFile config variable?  (Also
> > supported by the -O option to git-diff).
> 
> 
> I'll be happy to write a testcase, and to re-use the -O
> (diff.orderFile config var) option to git-diff as sugguested by John
> Keeping.
> 
> Is this the final spec?
> 
> 
> 
> I'll be happy to do that.
> 
> Luis

Hmm, I do like the idea of reusing the diff orderFile, but a
mechanism for sorting arbitrary inputs based on the orderFile
isn't currently exposed in a way that mergetool could use it.


Looking at the code in mergetool, we basically need something
that has the same spec as "sort" itself, namely that it can take
arbitrary arbitrary input on stdin and sort it.


Implementing the orderFile support would probably be best done
in C.  Would we want to expose it as an internal helper?

e.g.
git diff--order-helper 

could be used to perform the sorting.

But, that sort is honestly kinda crude.  It can't implement the
interesting case where we want bar.h to sort before bar.c but
not foo.h.

If we did the sort option, we could have both.

Thoughts?

-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] difftool: always honor "command not found" exit code

[David Aguilar]

On Sat, Aug 13, 2016 at 12:30:28PM +0100, John Keeping wrote:
> At the moment difftool's "trust exit code" logic always suppresses the
> exit status of the diff utility we invoke.  This is useful because we
> don't want to exit just because diff returned "1" because the files
> differ, but it's confusing if the shell returns an error because the
> selected diff utility is not found.
> 
> POSIX specifies 127 as the exit status for "command not found" and 126
> for "command found but is not executable" [1] and at least bash and dash
> follow this specification, while diff utilities generally use "1" for
> the exit status we want to ignore.
> 
> Handle 126 and 127 as special values, assuming that they always mean
> that the command could not be executed.
> 
> [1] 
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02
> 
> Signed-off-by: John Keeping <j...@keeping.me.uk>

Looks good to me, thanks.

Acked-by: David Aguilar <dav...@gmail.com>

> ---
> On Sat, Aug 13, 2016 at 11:36:39AM +0100, John Keeping wrote:
> > It would be nice if there was a way to differentiate between complete
> > failure and just the diff tool exiting with a non-zero return status
> > because the files differ, but I'm not sure whether we can do that
> > reliably.  POSIX uses 127 and 126 as errors that mean the command didn't
> > run [1] so it may be sensible to to treat those as special values.
> 
> Something like this perhaps?  I think this is probably safe, but it's
> always possible that some diff utility does use 126 or 127 as a "normal"
> exit status.  I'm not sure what we can do about that other than add a
> "really, really don't trust the exit status" option!


We can always add a mechanism for tool-specific error codes
later if we ever end up needing it, but this seems sufficient.

cheers,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-mergetool reverse file ordering

[David Aguilar]

On Wed, Jul 27, 2016 at 11:14:28AM +0100, Luis Gutierrez wrote:
> Hi,
> 
> Attached is a potential patch for reversing the order on which
> git-mergetool presents the files to merge.
> 
> Currently, when running git-mergetool, it performs a sort of the files
> to merge by alphabetical ordering. When working on C, this has the
> annoying effect of presenting the merge for a .c* files before the
> header files; which is always a bit harder to do. Reading the header
> first to figure out what the other dude changed is usually preferred.
> 
> The attach patch reverse the order (-r flag to sort) so *.h* are
> merged before  *.c* files
> 
> PS, given the simplicity of the patch, I have not tested it.
> 
> Regards
> 
> Luis

Thanks for the sug, this is an interesting idea and I definitely
see why we would want something like this...

> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index bf86270..cce3b0d 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -453,10 +453,10 @@ then
>   then
>   files=$(git rerere remaining)
>   else
> - files=$(git ls-files -u | sed -e 's/^[^ ]*  //' | sort -u)
> + files=$(git ls-files -u | sed -e 's/^[^ ]*  //' | sort -u 
> -r)
>   fi
>  else
> - files=$(git ls-files -u -- "$@" | sed -e 's/^[^ ]*  //' | sort -u)
> + files=$(git ls-files -u -- "$@" | sed -e 's/^[^ ]*  //' | sort -u 
> -r)
>  fi
>  
>  if test -z "$files"

While we won't take this patch as-is (please see
Documentation/SubmittingPatches for details about the patch
submission process), I am interested in the use case you've
described.

This use case makes me wonder whether the sorting we do here is
something that should be opened up a bit so that the it's not
quite so set in stone.

For example, an extension to the approach taken by this patch
would be to have `mergetool.reverseOrder` git config boolean
option that would tell us whether or not to use the "-r" flag
when calling sort.

But, IMO that is too rigid, and only addresses this narrow use
case.  What if users want a case-insensitive sort, or some other
preferred ordering?

We can address these concerns, and your use case, by opening it
up. Something like,

sort=$(git config mergetool.sort || echo sort -u)

That preserves the existing behavior, and it opens it up so that
we can accomplish the same result as this patch by doing:

git config mergetool.sort "sort -u -r"

Then, if someone later writes a nicer C/C++-specific sort that
sorts in the natural order but also keeps .h files before .c,
.cpp, etc. files with the same basename, then they could do:

git config mergetool.sort crescent-fresh-sort

...and it'll be totally crescent.

Thoughts?  Would you be interested in helping work up a patch
for this idea?  At a minimum we should also write a test case in
t/t7610-mergetool.sh to verify that it works as advertised.

Let me know if you have any questions.

cheers,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/4] subtree: adjust style to match CodingGuidelines

[David Aguilar]

Prefer "test" over "[ ... ]", use double-quotes around variables, break
long lines, and properly indent "case" statements.

Helped-by: Johannes Sixt <j...@kdbg.org>
Helped-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
This is a replacement patch for (previously) 3/3 that includes Junio's
suggestions to fix the alignment for continuation lines and case bodies.

The subsequent patch addresses the function definitions.

 contrib/subtree/git-subtree.sh | 575 +
 1 file changed, 357 insertions(+), 218 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index b567eae..de49eb0 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -4,8 +4,9 @@
 #
 # Copyright (C) 2009 Avery Pennarun <apenw...@gmail.com>
 #
-if [ $# -eq 0 ]; then
-set -- -h
+if test $# -eq 0
+then
+   set -- -h
 fi
 OPTS_SPEC="\
 git subtree add   --prefix= 
@@ -50,87 +51,146 @@ prefix=
 
 debug()
 {
-   if [ -n "$debug" ]; then
+   if test -n "$debug"
+   then
printf "%s\n" "$*" >&2
fi
 }
 
 say()
 {
-   if [ -z "$quiet" ]; then
+   if test -z "$quiet"
+   then
printf "%s\n" "$*" >&2
fi
 }
 
 progress()
 {
-   if [ -z "$quiet" ]; then
+   if test -z "$quiet"
+   then
printf "%s\r" "$*" >&2
fi
 }
 
 assert()
 {
-   if "$@"; then
-   :
-   else
+   if ! "$@"
+   then
die "assertion failed: " "$@"
fi
 }
 
 
-#echo "Options: $*"
-
-while [ $# -gt 0 ]; do
+while test $# -gt 0
+do
opt="$1"
shift
+
case "$opt" in
-   -q) quiet=1 ;;
-   -d) debug=1 ;;
-   --annotate) annotate="$1"; shift ;;
-   --no-annotate) annotate= ;;
-   -b) branch="$1"; shift ;;
-   -P) prefix="${1%/}"; shift ;;
-   -m) message="$1"; shift ;;
-   --no-prefix) prefix= ;;
-   --onto) onto="$1"; shift ;;
-   --no-onto) onto= ;;
-   --rejoin) rejoin=1 ;;
-   --no-rejoin) rejoin= ;;
-   --ignore-joins) ignore_joins=1 ;;
-   --no-ignore-joins) ignore_joins= ;;
-   --squash) squash=1 ;;
-   --no-squash) squash= ;;
-   --) break ;;
-   *) die "Unexpected option: $opt" ;;
+   -q)
+   quiet=1
+   ;;
+   -d)
+   debug=1
+   ;;
+   --annotate)
+   annotate="$1"
+   shift
+   ;;
+   --no-annotate)
+   annotate=
+   ;;
+   -b)
+   branch="$1"
+   shift
+   ;;
+   -P)
+   prefix="${1%/}"
+   shift
+   ;;
+   -m)
+   message="$1"
+   shift
+   ;;
+   --no-prefix)
+   prefix=
+   ;;
+   --onto)
+   onto="$1"
+   shift
+   ;;
+   --no-onto)
+   onto=
+   ;;
+   --rejoin)
+   rejoin=1
+   ;;
+   --no-rejoin)
+   rejoin=
+   ;;
+   --ignore-joins)
+   ignore_joins=1
+   ;;
+   --no-ignore-joins)
+   ignore_joins=
+   ;;
+   --squash)
+   squash=1
+   ;;
+   --no-squash)
+   squash=
+   ;;
+   --)
+   break
+   ;;
+   *)
+   die "Unexpected option: $opt"
+   ;;
esac
 done
 
 command="$1"
 shift
+
 case "$command" in
-   add|merge|pull) default= ;;
-   split|push) default="--default HEAD" ;;
-   *) die "Unknown command '$command'" ;;
+add|merge|pull)
+   default=
+   ;;
+split|push)
+   default="--default HEAD"
+   ;;
+*)
+   die "Unknown command '$command'"
+   ;;
 esac
 
-if [ -z "$prefix" ]; then
+if test -z "$prefix"
+then
die "You must provide the --prefix option."
 fi
 
 case "$command" in
-   add) [ -e "$prefix" ] && 
-   die "prefix '$prefix' already exists." ;;
-   *)   [ -e "$prefix" ] || 
-   die "'$prefix' does not exist; use 'git subtree add'" ;;
+add)
+   test -e "$pr

[PATCH v3 4/4] subtree: adjust function definitions to match CodingGuidelines

[David Aguilar]

We prefer a space between the function name and the parentheses, and no
space inside the parentheses.

The opening "{" should also be on the same line.

Suggested-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
This says "v3" but this is actually a branch new patch that wasn't included
in the previous iteration... calling it v3 since it's split out from the
previous patch.

They can be squashed together if desired, but this makes it a little
easier to review separately.

 contrib/subtree/git-subtree.sh | 102 ++---
 1 file changed, 34 insertions(+), 68 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index de49eb0..dec085a 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -49,32 +49,28 @@ squash=
 message=
 prefix=
 
-debug()
-{
+debug () {
if test -n "$debug"
then
printf "%s\n" "$*" >&2
fi
 }
 
-say()
-{
+say () {
if test -z "$quiet"
then
printf "%s\n" "$*" >&2
fi
 }
 
-progress()
-{
+progress () {
if test -z "$quiet"
then
printf "%s\r" "$*" >&2
fi
 }
 
-assert()
-{
+assert () {
if ! "$@"
then
die "assertion failed: " "$@"
@@ -202,8 +198,7 @@ debug "dir: {$dir}"
 debug "opts: {$*}"
 debug
 
-cache_setup()
-{
+cache_setup () {
cachedir="$GIT_DIR/subtree-cache/$$"
rm -rf "$cachedir" ||
die "Can't delete old cachedir: $cachedir"
@@ -214,8 +209,7 @@ cache_setup()
debug "Using cachedir: $cachedir" >&2
 }
 
-cache_get()
-{
+cache_get () {
for oldrev in "$@"
do
if test -r "$cachedir/$oldrev"
@@ -226,8 +220,7 @@ cache_get()
done
 }
 
-cache_miss()
-{
+cache_miss () {
for oldrev in "$@"
do
if ! test -r "$cachedir/$oldrev"
@@ -237,8 +230,7 @@ cache_miss()
done
 }
 
-check_parents()
-{
+check_parents () {
missed=$(cache_miss "$@")
for miss in $missed
do
@@ -249,13 +241,11 @@ check_parents()
done
 }
 
-set_notree()
-{
+set_notree () {
echo "1" > "$cachedir/notree/$1"
 }
 
-cache_set()
-{
+cache_set () {
oldrev="$1"
newrev="$2"
if test "$oldrev" != "latest_old" &&
@@ -267,8 +257,7 @@ cache_set()
echo "$newrev" >"$cachedir/$oldrev"
 }
 
-rev_exists()
-{
+rev_exists () {
if git rev-parse "$1" >/dev/null 2>&1
then
return 0
@@ -277,8 +266,7 @@ rev_exists()
fi
 }
 
-rev_is_descendant_of_branch()
-{
+rev_is_descendant_of_branch () {
newrev="$1"
branch="$2"
branch_hash=$(git rev-parse "$branch")
@@ -295,16 +283,14 @@ rev_is_descendant_of_branch()
 # if a commit doesn't have a parent, this might not work.  But we only want
 # to remove the parent from the rev-list, and since it doesn't exist, it won't
 # be there anyway, so do nothing in that case.
-try_remove_previous()
-{
+try_remove_previous () {
if rev_exists "$1^"
then
echo "^$1^"
fi
 }
 
-find_latest_squash()
-{
+find_latest_squash () {
debug "Looking for latest squash ($dir)..."
dir="$1"
sq=
@@ -348,8 +334,7 @@ find_latest_squash()
done
 }
 
-find_existing_splits()
-{
+find_existing_splits () {
debug "Looking for prior splits..."
dir="$1"
revs="$2"
@@ -393,8 +378,7 @@ find_existing_splits()
done
 }
 
-copy_commit()
-{
+copy_commit () {
# We're going to set some environment vars here, so
# do it in a subshell to get rid of them safely later
debug copy_commit "{$1}" "{$2}" "{$3}"
@@ -420,8 +404,7 @@ copy_commit()
) || die "Can't copy commit $1"
 }
 
-add_msg()
-{
+add_msg () {
dir="$1"
latest_old="$2"
latest_new="$3"
@@ -440,8 +423,7 @@ add_msg()
EOF
 }
 
-add_squashed_msg()
-{
+add_squashed_msg () {
if test -n "$message"
then
echo "$message"
@@ -450,8 +432,7 @@ add_squashed_msg()
fi
 }
 
-rejoin_msg()
-{
+rejoin_msg () {
dir="$1"
latest_old="$2"
latest_new="$3"
@@ -470,8 +451,7 @@ rejoin_msg()
EOF
 }
 
-squash_msg()
-{
+squash_msg () {
dir="$1"
 

[PATCH v2 3/3] subtree: adjust style to match CodingGuidelines

[David Aguilar]

Prefer "test" over "[ ... ]", use double-quotes around variables, break
long lines, and properly indent "case" statements.

Helped-by: Johannes Sixt <j...@kdbg.org>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
This is a replacement patch that addresses the notes from Hannes' review.

 contrib/subtree/git-subtree.sh | 570 +
 1 file changed, 354 insertions(+), 216 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index b567eae..9fd3b6a 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -4,8 +4,9 @@
 #
 # Copyright (C) 2009 Avery Pennarun <apenw...@gmail.com>
 #
-if [ $# -eq 0 ]; then
-set -- -h
+if test $# -eq 0
+then
+   set -- -h
 fi
 OPTS_SPEC="\
 git subtree add   --prefix= 
@@ -50,87 +51,145 @@ prefix=
 
 debug()
 {
-   if [ -n "$debug" ]; then
+   if test -n "$debug"
+   then
printf "%s\n" "$*" >&2
fi
 }
 
 say()
 {
-   if [ -z "$quiet" ]; then
+   if test -z "$quiet"
+   then
printf "%s\n" "$*" >&2
fi
 }
 
 progress()
 {
-   if [ -z "$quiet" ]; then
+   if test -z "$quiet"
+   then
printf "%s\r" "$*" >&2
fi
 }
 
 assert()
 {
-   if "$@"; then
-   :
-   else
+   if ! "$@"
+   then
die "assertion failed: " "$@"
fi
 }
 
 
-#echo "Options: $*"
-
-while [ $# -gt 0 ]; do
+while test $# -gt 0
+do
opt="$1"
shift
+
case "$opt" in
-   -q) quiet=1 ;;
-   -d) debug=1 ;;
-   --annotate) annotate="$1"; shift ;;
-   --no-annotate) annotate= ;;
-   -b) branch="$1"; shift ;;
-   -P) prefix="${1%/}"; shift ;;
-   -m) message="$1"; shift ;;
-   --no-prefix) prefix= ;;
-   --onto) onto="$1"; shift ;;
-   --no-onto) onto= ;;
-   --rejoin) rejoin=1 ;;
-   --no-rejoin) rejoin= ;;
-   --ignore-joins) ignore_joins=1 ;;
-   --no-ignore-joins) ignore_joins= ;;
-   --squash) squash=1 ;;
-   --no-squash) squash= ;;
-   --) break ;;
-   *) die "Unexpected option: $opt" ;;
+   -q)
+   quiet=1
+   ;;
+   -d)
+   debug=1
+   ;;
+   --annotate)
+   annotate="$1"
+   shift
+   ;;
+   --no-annotate)
+   annotate=
+   ;;
+   -b)
+   branch="$1"
+   shift
+   ;;
+   -P)
+   prefix="${1%/}"
+   shift
+   ;;
+   -m)
+   message="$1"
+   shift
+   ;;
+   --no-prefix)
+   prefix=
+   ;;
+   --onto)
+   onto="$1"
+   shift
+   ;;
+   --no-onto)
+   onto=
+   ;;
+   --rejoin)
+   rejoin=1
+   ;;
+   --no-rejoin)
+   rejoin=
+   ;;
+   --ignore-joins)
+   ignore_joins=1
+   ;;
+   --no-ignore-joins)
+   ignore_joins=
+   ;;
+   --squash)
+   squash=1
+   ;;
+   --no-squash)
+   squash=
+   ;;
+   --)
+   break
+   ;;
+   *)
+   die "Unexpected option: $opt"
+   ;;
esac
 done
 
 command="$1"
 shift
+
 case "$command" in
-   add|merge|pull) default= ;;
-   split|push) default="--default HEAD" ;;
-   *) die "Unknown command '$command'" ;;
+add|merge|pull)
+   default=
+   ;;
+split|push)
+   default="--default HEAD"
+   ;;
+*)
+   die "Unknown command '$command'"
+   ;;
 esac
 
-if [ -z "$prefix" ]; then
+if test -z "$prefix"
+then
die "You must provide the --prefix option."
 fi
 
 case "$command" in
-   add) [ -e "$prefix" ] && 
-   die "prefix '$prefix' already exists." ;;
-   *)   [ -e "$prefix" ] || 
-   die "'$prefix' does not exist; use 'git subtree add'" ;;
+add)
+   test -e "$prefix" && die "prefix '$prefix' already exists."
+   ;;
+*)
+   test -e "$prefix" ||
+   die "'$prefix' does not exist; use 'git

[PATCH 2/3] subtree: fix "git subtree split --rejoin"

[David Aguilar]

"git merge" in v2.9 prevents merging unrelated histories.

"git subtree split --rejoin" creates unrelated histories when
creating a split repo from a raw sub-directory that did not
originate from an invocation of "git subtree add".

Restore the original behavior by passing --allow-unrelated-histories
when merging subtrees.  This ensures that the synthetic history
created by "git subtree split" can be merged.

Add a test to ensure that this feature works as advertised.

Reported-by: Brett Cundal <brett.cun...@iugome.com>
Helped-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
This is a "re-implementation" of Brett's original RFC patch.
I preferred adding a new line (rather than modifying the existing line)
so I have no problem signing off on this being a distinct patch
authored by me.

 contrib/subtree/git-subtree.sh |  1 +
 contrib/subtree/t/t7900-subtree.sh | 16 
 2 files changed, 17 insertions(+)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7a39b30..b567eae 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -662,6 +662,7 @@ cmd_split()
debug "Merging split branch into HEAD..."
latest_old=$(cache_get latest_old)
git merge -s ours \
+   --allow-unrelated-histories \
-m "$(rejoin_msg "$dir" $latest_old $latest_new)" \
$latest_new >&2 || exit $?
fi
diff --git a/contrib/subtree/t/t7900-subtree.sh 
b/contrib/subtree/t/t7900-subtree.sh
index 431a2fe..e179b29 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -347,6 +347,22 @@ test_expect_success 'split sub dir/ with --rejoin' '
  '
 
 next_test
+test_expect_success 'split sub dir/ with --rejoin from scratch' '
+   subtree_test_create_repo "$subtree_test_count" &&
+   test_create_commit "$subtree_test_count" main1 &&
+   (
+   cd "$subtree_test_count" &&
+   mkdir "sub dir" &&
+   echo file >"sub dir"/file &&
+   git add "sub dir/file" &&
+   git commit -m"sub dir file" &&
+   split_hash=$(git subtree split --prefix="sub dir" --rejoin) &&
+   git subtree split --prefix="sub dir" --rejoin &&
+   check_equal "$(last_commit_message)" "Split '\''sub dir/'\'' 
into commit '\''$split_hash'\''"
+   )
+ '
+
+next_test
 test_expect_success 'split sub dir/ with --rejoin and --message' '
subtree_test_create_repo "$subtree_test_count" &&
subtree_test_create_repo "$subtree_test_count/sub proj" &&
-- 
2.9.2.466.g8c6d1f9.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] t7900-subtree.sh: fix quoting and broken && chains

[David Aguilar]

Allow whitespace in arguments to subtree_test_create_repo.
Add missing && chains.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 contrib/subtree/t/t7900-subtree.sh | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/subtree/t/t7900-subtree.sh 
b/contrib/subtree/t/t7900-subtree.sh
index 3bf96a9..431a2fe 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -16,16 +16,16 @@ export TEST_DIRECTORY
 
 subtree_test_create_repo()
 {
-   test_create_repo "$1"
+   test_create_repo "$1" &&
(
-   cd $1
+   cd "$1" &&
git config log.date relative
)
 }
 
 create()
 {
-   echo "$1" >"$1"
+   echo "$1" >"$1" &&
git add "$1"
 }
 
@@ -73,10 +73,10 @@ join_commits()
 test_create_commit() (
repo=$1
commit=$2
-   cd "$repo"
-   mkdir -p $(dirname "$commit") \
+   cd "$repo" &&
+   mkdir -p "$(dirname "$commit")" \
|| error "Could not create directory for commit"
-   echo "$commit" >"$commit"
+   echo "$commit" >"$commit" &&
git add "$commit" || error "Could not add commit"
git commit -m "$commit" || error "Could not commit"
 )
-- 
2.9.2.466.g8c6d1f9.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] subtree: adjust style to match CodingGuidelines

[David Aguilar]

Prefer "test" over "[ ... ]", use double-quotes around variables, break
long lines, and properly indent "case" statements.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 contrib/subtree/git-subtree.sh | 544 ++---
 1 file changed, 341 insertions(+), 203 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index b567eae..084c884 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -4,8 +4,9 @@
 #
 # Copyright (C) 2009 Avery Pennarun <apenw...@gmail.com>
 #
-if [ $# -eq 0 ]; then
-set -- -h
+if test $# -eq 0
+then
+   set -- -h
 fi
 OPTS_SPEC="\
 git subtree add   --prefix= 
@@ -50,87 +51,145 @@ prefix=
 
 debug()
 {
-   if [ -n "$debug" ]; then
-   printf "%s\n" "$*" >&2
+   if test -n "$debug"
+   then
+   printf "%s\n" "$@" >&2
fi
 }
 
 say()
 {
-   if [ -z "$quiet" ]; then
-   printf "%s\n" "$*" >&2
+   if test -z "$quiet"
+   then
+   printf "%s\n" "$@" >&2
fi
 }
 
 progress()
 {
-   if [ -z "$quiet" ]; then
-   printf "%s\r" "$*" >&2
+   if test -z "$quiet"
+   then
+   printf "%s\r" "$@" >&2
fi
 }
 
 assert()
 {
-   if "$@"; then
-   :
-   else
+   if ! "$@"
+   then
die "assertion failed: " "$@"
fi
 }
 
 
-#echo "Options: $*"
-
-while [ $# -gt 0 ]; do
+while test $# -gt 0
+do
opt="$1"
shift
+
case "$opt" in
-   -q) quiet=1 ;;
-   -d) debug=1 ;;
-   --annotate) annotate="$1"; shift ;;
-   --no-annotate) annotate= ;;
-   -b) branch="$1"; shift ;;
-   -P) prefix="${1%/}"; shift ;;
-   -m) message="$1"; shift ;;
-   --no-prefix) prefix= ;;
-   --onto) onto="$1"; shift ;;
-   --no-onto) onto= ;;
-   --rejoin) rejoin=1 ;;
-   --no-rejoin) rejoin= ;;
-   --ignore-joins) ignore_joins=1 ;;
-   --no-ignore-joins) ignore_joins= ;;
-   --squash) squash=1 ;;
-   --no-squash) squash= ;;
-   --) break ;;
-   *) die "Unexpected option: $opt" ;;
+   -q)
+   quiet=1
+   ;;
+   -d)
+   debug=1
+   ;;
+   --annotate)
+   annotate="$1"
+   shift
+   ;;
+   --no-annotate)
+   annotate=
+   ;;
+   -b)
+   branch="$1"
+   shift
+   ;;
+   -P)
+   prefix="${1%/}"
+   shift
+   ;;
+   -m)
+   message="$1"
+   shift
+   ;;
+   --no-prefix)
+   prefix=
+   ;;
+   --onto)
+   onto="$1"
+   shift
+   ;;
+   --no-onto)
+   onto=
+   ;;
+   --rejoin)
+   rejoin=1
+   ;;
+   --no-rejoin)
+   rejoin=
+   ;;
+   --ignore-joins)
+   ignore_joins=1
+   ;;
+   --no-ignore-joins)
+   ignore_joins=
+   ;;
+   --squash)
+   squash=1
+   ;;
+   --no-squash)
+   squash=
+   ;;
+   --)
+   break
+   ;;
+   *)
+   die "Unexpected option: $opt"
+   ;;
esac
 done
 
 command="$1"
 shift
+
 case "$command" in
-   add|merge|pull) default= ;;
-   split|push) default="--default HEAD" ;;
-   *) die "Unknown command '$command'" ;;
+add|merge|pull)
+   default=
+   ;;
+split|push)
+   default="--default HEAD"
+   ;;
+*)
+   die "Unknown command '$command'"
+   ;;
 esac
 
-if [ -z "$prefix" ]; then
+if test -z "$prefix"
+then
die "You must provide the --prefix option."
 fi
 
 case "$command" in
-   add) [ -e "$prefix" ] && 
-   die "prefix '$prefix' already exists." ;;
-   *)   [ -e "$prefix" ] || 
-   die "'$prefix' does not exist; use 'git subtree add'" ;;
+add)
+   test -e "$prefix" && die "prefix '$prefix' already exists."
+   ;;
+*)
+   test -e &

Re: [PATCH] git-subtree.sh: Use --allow-unrelated-histories when splitting with --rejoin

[David Aguilar]

[cc'd Roberto for submitGit q's]

On Thu, Jul 21, 2016 at 12:56:51AM +, Brett Cundal wrote:
> ---



The message on the pull request[1] has a better justification
for this change, which would have been nice in the commit
message itself:


Git 2.9 added a check against merging unrelated histories, which
is exactly what git subtree with --rejoin does. Adding the
--allow-unrelated-histories flag to merge will override this
check.


Is it possible that maybe submitGit can detect an empty commit
message for single-commit PRs and transplant that message onto it?

As-is, the commit itself should probably be amended to contain
that information.

>  contrib/subtree/git-subtree.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 7a39b30..556cd92 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -661,7 +661,7 @@ cmd_split()
>   if [ -n "$rejoin" ]; then
>   debug "Merging split branch into HEAD..."
>   latest_old=$(cache_get latest_old)
> - git merge -s ours \
> + git merge -s ours --allow-unrelated-histories \
>   -m "$(rejoin_msg "$dir" $latest_old $latest_new)" \
>   $latest_new >&2 || exit $?
>   fi
> 
> --

With the above description this change makes more sense,
but it seems that the existing tests do not detect the breakage
fixed by this patch.

Can you please add a test case in t/t7900-subtree.sh
demonstrating the breakage?

Looks good otherwise.

[1] https://github.com/git/git/pull/274
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] difftool: avoid $GIT_DIR and $GIT_WORK_TREE

[David Aguilar]

On Tue, Jul 19, 2016 at 02:06:35PM -0700, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > It is not wrong per-se, but as you are in a subshell, you do not
> > have to unset these, I would think.  Not worth a reroll, but unless
> > I am overlooking something (in which case please holler) I'm
> > inclined to remove these two lines myself while queuing the series.
> 
> I propose to squashing the following to 2/3 (and adjusting 3/3 as
> needed).  No need to resend if you agree it is a good idea, as it is
> part of what I've queued on 'pu'.
> 
> Thanks.


I had originally meant to squash that in but it slipped through.
It looks great.

Thank you!


>  git-difftool.perl   | 2 +-
>  t/t7800-difftool.sh | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/git-difftool.perl b/git-difftool.perl
> index bc2267f..c81cbe4 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -88,11 +88,11 @@ sub changed_files
>   my @refreshargs = (
>   @gitargs, 'update-index',
>   '--really-refresh', '-q', '--unmerged');
> - my @diffargs = (@gitargs, 'diff-files', '--name-only', '-z');
>   try {
>   Git::command_oneline(@refreshargs);
>   } catch Git::Error::Command with {};
>  
> + my @diffargs = (@gitargs, 'diff-files', '--name-only', '-z');
>   my $line = Git::command_oneline(@diffargs);
>   my @files;
>   if (defined $line) {
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index afdf370..cb25480 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -421,8 +421,6 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory 
> with GIT_DIR set' '
>   cd sub &&
>   git difftool --dir-diff $symlinks --extcmd ls \
>   branch -- sub >output &&
> - sane_unset GIT_WORK_TREE &&
> - sane_unset GIT_DIR &&
>   grep sub output &&
>   ! grep file output
>   )
> -- 
> 2.9.2-581-g77f0ffb
> 

-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] difftool: fix argument handling in subdirs

[David Aguilar]

From: John Keeping <j...@keeping.me.uk>

When in a subdirectory of a repository, path arguments should be
interpreted relative to the current directory not the root of the
working tree.

The Git::repository object passed into setup_dir_diff() is configured to
handle this correctly but we create a new Git::repository here without
setting the WorkingSubdir argument.  By simply using the existing
repository, path arguments are handled relative to the current
directory.

Reported-by: Bernhard Kirchen <bernhard.kirc...@rwth-aachen.de>
Signed-off-by: John Keeping <j...@keeping.me.uk>
Acked-by: David Aguilar <dav...@gmail.com>
---
This patch is unchanged from John's version but also includes
Reported-by and Acked-by lines.

 git-difftool.perl | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index ebd13ba..c9d3ef8 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -115,16 +115,9 @@ sub setup_dir_diff
 {
my ($repo, $workdir, $symlinks) = @_;
 
-   # Run the diff; exit immediately if no diff found
-   # 'Repository' and 'WorkingCopy' must be explicitly set to insure that
-   # if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used
-   # by Git->repository->command*.
my $repo_path = $repo->repo_path();
-   my %repo_args = (Repository => $repo_path, WorkingCopy => $workdir);
-   my $diffrepo = Git->repository(%repo_args);
-
my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
-   my $diffrtn = $diffrepo->command_oneline(@gitargs);
+   my $diffrtn = $repo->command_oneline(@gitargs);
exit(0) unless defined($diffrtn);
 
# Build index info for left and right sides of the diff
@@ -176,12 +169,12 @@ EOF
 
if ($lmode eq $symlink_mode) {
$symlink{$src_path}{left} =
-   $diffrepo->command_oneline('show', "$lsha1");
+   $repo->command_oneline('show', "$lsha1");
}
 
if ($rmode eq $symlink_mode) {
$symlink{$dst_path}{right} =
-   $diffrepo->command_oneline('show', "$rsha1");
+   $repo->command_oneline('show', "$rsha1");
}
 
if ($lmode ne $null_mode and $status !~ /^C/) {
-- 
2.9.2.280.g385e27a

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] difftool: use Git::* functions instead of passing around state

[David Aguilar]

Call Git::command() and friends directly wherever possible.
This makes it clear that these operations can be invoked directly
without needing to manage the current directory and related GIT_*
environment variables.

Eliminate find_repository() since we can now use wc_path() and
not worry about side-effects involving environment variables.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-difftool.perl | 54 ++
 1 file changed, 22 insertions(+), 32 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index bc2267f..a5790d0 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -37,14 +37,6 @@ USAGE
exit($exitcode);
 }
 
-sub find_worktree
-{
-   # Git->repository->wc_path() does not honor changes to the working
-   # tree location made by $ENV{GIT_WORK_TREE} or the 'core.worktree'
-   # config variable.
-   return Git::command_oneline('rev-parse', '--show-toplevel');
-}
-
 sub print_tool_help
 {
# See the comment at the bottom of file_diff() for the reason behind
@@ -67,14 +59,14 @@ sub exit_cleanup
 
 sub use_wt_file
 {
-   my ($repo, $workdir, $file, $sha1) = @_;
+   my ($workdir, $file, $sha1) = @_;
my $null_sha1 = '0' x 40;
 
if (-l "$workdir/$file" || ! -e _) {
return (0, $null_sha1);
}
 
-   my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
+   my $wt_sha1 = Git::command_oneline('hash-object', "$workdir/$file");
my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
return ($use, $wt_sha1);
 }
@@ -88,11 +80,11 @@ sub changed_files
my @refreshargs = (
@gitargs, 'update-index',
'--really-refresh', '-q', '--unmerged');
-   my @diffargs = (@gitargs, 'diff-files', '--name-only', '-z');
try {
Git::command_oneline(@refreshargs);
} catch Git::Error::Command with {};
 
+   my @diffargs = (@gitargs, 'diff-files', '--name-only', '-z');
my $line = Git::command_oneline(@diffargs);
my @files;
if (defined $line) {
@@ -108,11 +100,9 @@ sub changed_files
 
 sub setup_dir_diff
 {
-   my ($repo, $workdir, $symlinks) = @_;
-
-   my $repo_path = $repo->repo_path();
+   my ($workdir, $symlinks) = @_;
my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
-   my $diffrtn = $repo->command_oneline(@gitargs);
+   my $diffrtn = Git::command_oneline(@gitargs);
exit(0) unless defined($diffrtn);
 
# Build index info for left and right sides of the diff
@@ -164,12 +154,12 @@ EOF
 
if ($lmode eq $symlink_mode) {
$symlink{$src_path}{left} =
-   $repo->command_oneline('show', "$lsha1");
+   Git::command_oneline('show', $lsha1);
}
 
if ($rmode eq $symlink_mode) {
$symlink{$dst_path}{right} =
-   $repo->command_oneline('show', "$rsha1");
+   Git::command_oneline('show', $rsha1);
}
 
if ($lmode ne $null_mode and $status !~ /^C/) {
@@ -181,8 +171,8 @@ EOF
if ($working_tree_dups{$dst_path}++) {
next;
}
-   my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
- $dst_path, $rsha1);
+   my ($use, $wt_sha1) =
+   use_wt_file($workdir, $dst_path, $rsha1);
if ($use) {
push @working_tree, $dst_path;
$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
@@ -203,27 +193,27 @@ EOF
my ($inpipe, $ctx);
$ENV{GIT_INDEX_FILE} = "$tmpdir/lindex";
($inpipe, $ctx) =
-   $repo->command_input_pipe(qw(update-index -z --index-info));
+   Git::command_input_pipe('update-index', '-z', '--index-info');
print($inpipe $lindex);
-   $repo->command_close_pipe($inpipe, $ctx);
+   Git::command_close_pipe($inpipe, $ctx);
 
my $rc = system('git', 'checkout-index', '--all', "--prefix=$ldir/");
exit_cleanup($tmpdir, $rc) if $rc != 0;
 
$ENV{GIT_INDEX_FILE} = "$tmpdir/rindex";
($inpipe, $ctx) =
-   $repo->command_input_pipe(qw(update-index -z --index-info));
+   Git::command_input_pipe('update-index', '-z', '--index-info');
print($inpipe $rindex);
-   $repo->command_close_pipe($inpipe, $ctx);
+   Git::command_close_pipe($inpipe, $ctx);
 
$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
exit_cleanup($tmpdir, $

[PATCH 2/3] difftool: avoid $GIT_DIR and $GIT_WORK_TREE

[David Aguilar]

Environment variables are global and hard to reason about.
Use the `--git-dir` and `--work-tree` arguments when invoking `git`
instead of relying on the environment.

Add a test to ensure that difftool's dir-diff feature works when these
variables are present in the environment.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-difftool.perl   | 27 ++-
 t/t7800-difftool.sh | 16 
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index c9d3ef8..bc2267f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -83,20 +83,17 @@ sub changed_files
 {
my ($repo_path, $index, $worktree) = @_;
$ENV{GIT_INDEX_FILE} = $index;
-   $ENV{GIT_WORK_TREE} = $worktree;
-   my $must_unset_git_dir = 0;
-   if (not defined($ENV{GIT_DIR})) {
-   $must_unset_git_dir = 1;
-   $ENV{GIT_DIR} = $repo_path;
-   }
 
-   my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
-   my @gitargs = qw/diff-files --name-only -z/;
+   my @gitargs = ('--git-dir', $repo_path, '--work-tree', $worktree);
+   my @refreshargs = (
+   @gitargs, 'update-index',
+   '--really-refresh', '-q', '--unmerged');
+   my @diffargs = (@gitargs, 'diff-files', '--name-only', '-z');
try {
Git::command_oneline(@refreshargs);
} catch Git::Error::Command with {};
 
-   my $line = Git::command_oneline(@gitargs);
+   my $line = Git::command_oneline(@diffargs);
my @files;
if (defined $line) {
@files = split('\0', $line);
@@ -105,8 +102,6 @@ sub changed_files
}
 
delete($ENV{GIT_INDEX_FILE});
-   delete($ENV{GIT_WORK_TREE});
-   delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
 
return map { $_ => 1 } @files;
 }
@@ -204,15 +199,6 @@ EOF
mkpath($ldir) or exit_cleanup($tmpdir, 1);
mkpath($rdir) or exit_cleanup($tmpdir, 1);
 
-   # If $GIT_DIR is not set prior to calling 'git update-index' and
-   # 'git checkout-index', then those commands will fail if difftool
-   # is called from a directory other than the repo root.
-   my $must_unset_git_dir = 0;
-   if (not defined($ENV{GIT_DIR})) {
-   $must_unset_git_dir = 1;
-   $ENV{GIT_DIR} = $repo_path;
-   }
-
# Populate the left and right directories based on each index file
my ($inpipe, $ctx);
$ENV{GIT_INDEX_FILE} = "$tmpdir/lindex";
@@ -241,7 +227,6 @@ EOF
 
# If $GIT_DIR was explicitly set just for the update/checkout
# commands, then it should be unset before continuing.
-   delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
delete($ENV{GIT_INDEX_FILE});
 
# Changes in the working tree need special treatment since they are
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 42a2929..fa43c24 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -412,6 +412,22 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '
)
 '
 
+run_dir_diff_test 'difftool --dir-diff from subdirectory with GIT_DIR set' '
+   (
+   GIT_DIR=$(pwd)/.git &&
+   export GIT_DIR &&
+   GIT_WORK_TREE=$(pwd) &&
+   export GIT_WORK_TREE &&
+   cd sub &&
+   git difftool --dir-diff $symlinks --extcmd ls \
+   branch -- sub >output &&
+   sane_unset GIT_WORK_TREE &&
+   sane_unset GIT_DIR &&
+   grep sub output &&
+   ! grep file output
+   )
+'
+
 run_dir_diff_test 'difftool --dir-diff when worktree file is missing' '
test_when_finished git reset --hard &&
rm file2 &&
-- 
2.9.2.280.g385e27a

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] difftool: fix argument handling in subdirs

[David Aguilar]

[Cc'd Tim, who originally authored the dir-diff code]

On Tue, Jul 05, 2016 at 08:52:52PM +0100, John Keeping wrote:
> On Mon, Jul 04, 2016 at 08:37:39PM +0200, Bernhard Kirchen wrote:
> > Today I started using --dir-diff and noticed a problem when specifying a
> > non-full path limiter. My diff tool is setup to be meld (*1).
> > 
> > OK while working directory is repo root; also OK while working directory is
> > repo subfolder "actual":
> > git difftool --dir-diff HEAD~1 HEAD -- actual/existing/path
> > => meld opens with proper dir-diff.
> > 
> > NOT OK while working directory is repo subfolder "actual":
> > git difftool --dir-diff HEAD~1 HEAD -- existing/path
> > => nothing happens, as if using "non/such/path" as the path limiter.
> > 
> > Because "git diff HEAD~1 HEAD -- existing/path" while the working directory
> > is the repo subfolder "actual" works, I epxected the difftool to work
> > similarly. Is this a bug?
> 
> I think it is, yes.  The patch below fixes it for me and doesn't break
> any existing tests, but I still don't understand why the separate
> $diffrepo was needed originally, so I'm not certain this won't break
> some other corner case.


IIRC the original motivation for using a separate $diffrepo was
to handle GIT_DIR being set in the environment.

The lack of tests for that use case could be better, though.
Is that use case affected by this change?

Tim, do you remember why a new repo instance is used for that code path?


> -- >8 --
> When in a subdirectory of a repository, path arguments should be
> interpreted relative to the current directory not the root of the
> working tree.
> 
> The Git::repository object passed into setup_dir_diff() is configured to
> handle this correctly but we create a new Git::repository here without
> setting the WorkingSubdir argument.  By simply using the existing
> repository, path arguments are handled relative to the current
> directory.

I do like the sound of this rationale, though.

Tim, please let us know if you have a specific test case that is
not covered by this change.

BTW, `diff --raw` will still output paths that are relative to
the root but this is okay since the rest of the code expects
things to be root-relative, correct?


> Signed-off-by: John Keeping 
> ---
>  git-difftool.perl | 13 +++--
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/git-difftool.perl b/git-difftool.perl
> index ebd13ba..c9d3ef8 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -115,16 +115,9 @@ sub setup_dir_diff
>  {
>   my ($repo, $workdir, $symlinks) = @_;
>  
> - # Run the diff; exit immediately if no diff found
> - # 'Repository' and 'WorkingCopy' must be explicitly set to insure that
> - # if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used
> - # by Git->repository->command*.
>   my $repo_path = $repo->repo_path();
> - my %repo_args = (Repository => $repo_path, WorkingCopy => $workdir);
> - my $diffrepo = Git->repository(%repo_args);
> -
>   my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
> - my $diffrtn = $diffrepo->command_oneline(@gitargs);
> + my $diffrtn = $repo->command_oneline(@gitargs);
>   exit(0) unless defined($diffrtn);
>  
>   # Build index info for left and right sides of the diff
> @@ -176,12 +169,12 @@ EOF
>  
>   if ($lmode eq $symlink_mode) {
>   $symlink{$src_path}{left} =
> - $diffrepo->command_oneline('show', "$lsha1");
> + $repo->command_oneline('show', "$lsha1");
>   }
>  
>   if ($rmode eq $symlink_mode) {
>   $symlink{$dst_path}{right} =
> - $diffrepo->command_oneline('show', "$rsha1");
> + $repo->command_oneline('show', "$rsha1");
>   }
>  
>   if ($lmode ne $null_mode and $status !~ /^C/) {
> -- 

Can you please also add a testcase to t/t7800-difftool.sh
demonstrating the problem fixed by this change?

Hopefully there's an existing test in there that can be adapted
to run dir-diffs in a subdirectory.

ciao,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] i18n: difftool: mark warnings for translation

[David Aguilar]

On Tue, Jun 21, 2016 at 11:44:13AM +, Vasco Almeida wrote:
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -451,11 +452,11 @@ sub dir_diff
>   }
>  
>   if (exists $wt_modified{$file} and exists $tmp_modified{$file}) 
> {
> - my $errmsg = "warning: Both files modified: ";
> - $errmsg .= "'$workdir/$file' and '$b/$file'.\n";
> - $errmsg .= "warning: Working tree file has been 
> left.\n";
> - $errmsg .= "warning:\n";
> - warn $errmsg;
> + warn sprintf __(
> +"warning: Both files modified:
> +'%s/%s' and '%s/%s'.
> +warning: Working tree file has been left.
> +warning:\n"), $workdir, $file, $b, $file;


Sorry for the nit, but this seems a little hard to read.

It's unfortunate that we have to lose the flow of the code here.
Perhaps we can do something like this to make it flow more like
the original instead?

+   warn sprintf(__(
+   "warning: Both files modified:\n" .
+   "'%s/%s' and '%s/%s'.\n" .
+   "warning: Working tree file has been left.\n" .
+   "warning:\n"), $workdir, $file, $b, $file);

I tend to also prefer parens on the sprintf so that the
parameter grouping is easier to see.

> @@ -467,8 +468,9 @@ sub dir_diff
>   }
>   }
>   if ($error) {
> - warn "warning: Temporary files exist in '$tmpdir'.\n";
> - warn "warning: You may want to cleanup or recover these.\n";
> + warn sprintf __(
> +"warning: Temporary files exist in '%s'.
> +warning: You may want to cleanup or recover these.\n"), $tmpdir;

Ditto, perhaps something like...

+   warn sprintf(__(
+   "warning: Temporary files exist in '%s'.\n" .
+   "warning: You may want to cleanup or recover these.\n"
+   ), $tmpdir);

I'm assuming that the i18n infrastructure is prepared to deal
with perl's . dot syntax for string concatenation, though, and
I don't know whether that's true.  Does that work?

What do you think?

ciao,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t7610-mergetool.sh test failure

[David Aguilar]

On Wed, May 25, 2016 at 08:51:14PM -0500, Jeff King wrote:
> On Wed, May 25, 2016 at 06:16:15PM -0500, Jeff King wrote:
> 
> > On Tue, May 24, 2016 at 09:45:25AM -0700, Junio C Hamano wrote:
> > 
> > > On Tue, May 24, 2016 at 9:44 AM, Armin Kunaschik
> > >  wrote:
> > > > t7610-mergetool.sh fails on systems without mktemp.
> > > >
> > > > mktemp is used in git-mergetool.sh and throws an error when it's not 
> > > > available.
> > > > error: mktemp is needed when 'mergetool.writeToTemp' is true
> > > >
> > > > I see 2 options:
> > > > 1. code around it, write an own mktemp, maybe use the test-mktemp as a 
> > > > basis.
> > > > 2. disable the test when mktemp is not available
> > > 
> > > 3. find and install mktemp?
> > 
> > I'd agree more with (3) if it was some critical part of git-mergetool.
> > But it seems to be a completely optional feature that we use in only one
> > place, and git-mergetool even detects this case and complains.
> > 
> > So another alternative would be for the test to detect either that
> > mergetool worked, or that we got the "mktemp is needed" error.
> 
> BTW, one thing I happened to note while looking at this: running the
> test script will write into /tmp (or wherever your $TMPDIR points).
> Probably not a big deal, but I wonder if we should be setting $TMPDIR in
> our test harness.

We already set $HOME and various other variables to carefully
tune the testsuite's behavior, so this sounds like a good idea
IMO.

Would there be any downsides to setting $TMPDIR equal to the
trash directory?

FWIW I set $TMPDIR to the $TEST_OUTPUT_DIRECTORY in test-lib.sh
and was able to run the test suite without any errors.

Is it worth patching?
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t7800 test failure

[David Aguilar]

On Wed, May 25, 2016 at 11:33:33AM +0200, Armin Kunaschik wrote:
> On Tue, May 24, 2016 at 7:36 PM, Junio C Hamano <gits...@pobox.com> wrote:
> > Armin Kunaschik <megabr...@googlemail.com> writes:
> >>
> >> Ok, how can this be implemented within the test environment?
> >
> > I actually think an unconditional check like this is sufficient.
> 
> Ah, good. My thoughts were a bit more complicated.
> Anyway, this works for me.
> Thanks!

Would you mind submitting a patch so that we can support these
tests when running on AIX/HP-UX?


> >  t/t7800-difftool.sh | 17 +
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> > index 7ce4cd7..f304228 100755
> > --- a/t/t7800-difftool.sh
> > +++ b/t/t7800-difftool.sh
> > @@ -442,15 +442,16 @@ run_dir_diff_test 'difftool --dir-diff with unmerged 
> > files' '
> > test_cmp expect actual
> >  '
> >
> > -write_script .git/CHECK_SYMLINKS <<\EOF
> > -for f in file file2 sub/sub
> > -do
> > -   echo "$f"
> > -   readlink "$2/$f"
> > -done >actual
> > -EOF
> > -
> >  test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without 
> > unstaged changes' '
> > +
> > +   write_script .git/CHECK_SYMLINKS <<-\EOF &&
> > +   for f in file file2 sub/sub
> > +   do
> > +   echo "$f"
> > +   ls -ld "$2/$f" | sed -e "s/.* -> //"
> > +   done >actual
> > +   EOF
> > +
> > cat >expect <<-EOF &&
> > file
> > $PWD/file

I was curious so I whipped together a small tweak to
t/check-non-portable-shell.pl below.

The difftool tests are not the only ones that use readlink.
My guess is you haven't run the p4 tests because AIX/HP-UX doesn't have p4?

$ make test-lint-shell-syntax
t7800-difftool.sh:449: error: readlink is not portable (please use ls 
-ld | sed):   readlink "$2/$f"
t9802-git-p4-filetype.sh:266: error: readlink is not portable (please 
use ls -ld | sed):test $(readlink symlink) = symlink-target
t9802-git-p4-filetype.sh:332: error: readlink is not portable (please 
use ls -ld | sed):test $(readlink empty-symlink) = target2
test-lib.sh:757: error: readlink is not portable (please use ls -ld | 
sed): test "$1" = "$(readlink "$2")" || {

If we want to ban use of readlink from the test suite we could
add it to the check script.  test-lib.sh also includes it for
valgrind support so I'm not really sure whether we'd want to
apply this, but I figured I'd bring it up for discussion.

If we end up fixing all of these then I can send this to the
list as a proper patch.

Curious, is there an easy way to get readlink and mktemp installed on AIX?

Another alternative is that we can compile our own
"git-readlink" and "git-mktemp" programs and use those instead,
but that seems like a big maintenance burden compared to the
relative simplicity of the test-suite workarounds.

Thanks for fixing my non-portable tests ;-)

--- 8< --- 8< ---
>From 40c41402dfa24ca16b41062172c34f238d77b42c Mon Sep 17 00:00:00 2001
From: David Aguilar <dav...@gmail.com>
Date: Thu, 26 May 2016 02:42:52 -0700
Subject: [PATCH] tests: add shell portability check for "readlink"

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 t/check-non-portable-shell.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b170cbc..2707e42 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -18,6 +18,7 @@ while (<>) {
chomp;
/\bsed\s+-i/ and err 'sed -i is not portable';
/\becho\s+-n/ and err 'echo -n is not portable (please use printf)';
+   /\breadlink\s+/ and err 'readlink is not portable (please use ls -ld | 
sed)';
/^\s*declare\s+/ and err 'arrays/declare not portable';
/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
/\btest\s+[^=]*==/ and err '"test a == b" is not portable (please use 
=)';
-- 
2.7.0.rc3

-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] difftool: handle unmerged files in dir-diff mode

[David Aguilar]

When files are unmerged they can show up as both unmerged and
modified in the output of `git diff --raw`.  This causes
difftool's dir-diff to create filesystem entries for the same
path twice, which fails when it encounters a duplicate path.

Ensure that each worktree path is only processed once.
Add a test to demonstrate the breakage.

Reported-by: Jan Smets <j...@smets.cx>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-difftool.perl   |  5 +
 t/t7800-difftool.sh | 23 +++
 2 files changed, 28 insertions(+)

diff --git a/git-difftool.perl b/git-difftool.perl
index 8cf0040..ebd13ba 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -138,6 +138,7 @@ sub setup_dir_diff
my %submodule;
my %symlink;
my @working_tree = ();
+   my %working_tree_dups = ();
my @rawdiff = split('\0', $diffrtn);
 
my $i = 0;
@@ -188,6 +189,10 @@ EOF
}
 
if ($rmode ne $null_mode) {
+   # Avoid duplicate working_tree entries
+   if ($working_tree_dups{$dst_path}++) {
+   next;
+   }
my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
  $dst_path, $rsha1);
if ($use) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ff7a9e9..7ce4cd7 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -419,6 +419,29 @@ run_dir_diff_test 'difftool --dir-diff when worktree file 
is missing' '
grep file2 output
 '
 
+run_dir_diff_test 'difftool --dir-diff with unmerged files' '
+   test_when_finished git reset --hard &&
+   test_config difftool.echo.cmd "echo ok" &&
+   git checkout -B conflict-a &&
+   git checkout -B conflict-b &&
+   git checkout conflict-a &&
+   echo a >>file &&
+   git add file &&
+   git commit -m conflict-a &&
+   git checkout conflict-b &&
+   echo b >>file &&
+   git add file &&
+   git commit -m conflict-b &&
+   git checkout master &&
+   git merge conflict-a &&
+   test_must_fail git merge conflict-b &&
+   cat >expect <<-EOF &&
+   ok
+   EOF
+   git difftool --dir-diff $symlinks -t echo >actual &&
+   test_cmp expect actual
+'
+
 write_script .git/CHECK_SYMLINKS <<\EOF
 for f in file file2 sub/sub
 do
-- 
2.8.2.404.gdfc6a507

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] difftool: initialize variables for readability

[David Aguilar]

The code always goes into one of the two conditional blocks but make it
clear that not doing so is an error condition by setting $ok to 0.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 git-difftool.perl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 488d14b..8cf0040 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -273,7 +273,7 @@ EOF
# temporary file to both the left and right directories to show the
# change in the recorded SHA1 for the submodule.
for my $path (keys %submodule) {
-   my $ok;
+   my $ok = 0;
if (defined($submodule{$path}{left})) {
$ok = write_to_file("$ldir/$path",
"Subproject commit $submodule{$path}{left}");
@@ -289,7 +289,7 @@ EOF
# shows only the link itself, not the contents of the link target.
# This loop replicates that behavior.
for my $path (keys %symlink) {
-   my $ok;
+   my $ok = 0;
if (defined($symlink{$path}{left})) {
$ok = write_to_file("$ldir/$path",
$symlink{$path}{left});
-- 
2.8.2.404.gdfc6a507

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: using git-difftool -d when cherry-picking

[David Aguilar]

On Wed, Apr 27, 2016 at 11:12:25AM +0200, Jan Smets wrote:
> Hi
> 
> Please consider following example
> 
> #!/bin/bash
> rm -rf /tmp/gittest
> mkdir /tmp/gittest
> cd /tmp/gittest
> 
> git init
> 
> echo $RANDOM > testfile
> git add testfile
> git commit -m test -a
> 
> git branch X
> git checkout X
> echo $RANDOM > testfile
> git add testfile
> git commit -m test -a
> 
> git checkout master
> echo $RANDOM > testfile
> git add testfile
> git commit -m test -a
> 
> git cherry-pick X
> git diff --raw
> git difftool -d
> 
> 
> This emulates a merge conflict when using git-cerry-pick.
> 
> $ git diff --raw
> :00 100644 000... 000... U  testfile
> :100644 100644 a04e026... 000... M  testfile
> 
> When executing git difftool with the -d option :
> 
> /usr/lib/git-core/git-difftool line 260: File exists
> 
> A possible solution is to build an unique list in @working_tree
> 
> The purpose is to edit/resolve the conflict in the difftool.


That could be useful.  git-mergetool is intended to be used when
merge conflicts exist, but it sounds like you may have already
found a possible solution by making @working_tree unique.  Have
you tested that to see if it skirts around the issue?

If you have a patch I'd be happy to help review and test it.
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] difftool/mergetool: make the form of yes/no questions consistent

[David Aguilar]

On Tue, Apr 12, 2016 at 02:15:09PM -0700, Junio C Hamano wrote:
> Nikola Forró  writes:
> 
> > Every yes/no question in difftool/mergetool scripts has slightly
> > different form, and none of them is consistent with the form git
> > itself uses.
> >
> > Make the form of all the questions consistent with the form used
> > by git.
> >
> > Reviewed-by: John Keeping 
> > Signed-off-by: Nikola Forró 
> > ---
> > Changes in v2: example dropped from the commit message
> 
> Thanks; have you run the test suite with this patch applied?
> 
> It is your responsibility to make sure that the expectation by
> existing tests are still satisfied after your change, or update
> their expectation to match the new (and hopefully better) world
> order your patch introduces.
> 
> I needed to squash this in to make the tests pass, but because I am
> not a difftool user, I do not at all know if the prompt produced
> (and expected by the test) is sensible or not.
> 
>  t/t7800-difftool.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index ec8bc8c..df9050f 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -20,7 +20,7 @@ difftool_test_setup ()
>  prompt_given ()
>  {
>   prompt="$1"
> - test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
> + test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
>  }
>  
>  # Create a file on master and change it on branch

That looks correct to me.  Sorry 'bout that, I'll remember to
run the tests with the patches applied next time.

cheers,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent

[David Aguilar]

On Tue, Apr 12, 2016 at 03:01:19PM +0100, John Keeping wrote:
> On Tue, Apr 12, 2016 at 03:53:12PM +0200, Nikola Fo wrote:
> > On Tue, 2016-04-12 at 14:27 +0100, John Keeping wrote:
> > > I think the case in these two is correct as-is.  The "Y" is capitalised
> > > because it is the default and will take effect if the user just presses
> > > ENTER.
> > 
> > Thanks John, I'm aware of that. That's why the patch doesn't change
> > the case. Maybe I should have mention that explicitly in the commit
> > message.
> 
> Sorry, I completely missed that.  Your patch does in fact look good, so:
> 
>   Reviewed-by: John Keeping <j...@keeping.me.uk>
> 
> I think I was taken in by the commit message saying 'i.e. "Question
> [y/n]? "' and didn't examine the patch carefully enough.  It might be
> better just to drop the example since it's obvious what the patch does.

Thanks for reviewing.

Acked-by: David Aguilar <dav...@gmail.com>


cheers,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] mergetools: add support for ExamDiff

[David Aguilar]

On Fri, Apr 01, 2016 at 11:10:56AM -0700, Junio C Hamano wrote:
> Jacob Nisnevich <jacob.nisnev...@gmail.com> writes:
> 
> > OK I add the quotes and modified the comment. I also changed $folder to 
> > $sub_directory. I think that makes a little bit more sense and sounds a lot
> > better.
> >
> > Jacob Nisnevich (2):
> >   mergetools: create mergetool_find_win32_cmd() helper function for
> > winmerge
> >   mergetools: add support for ExamDiff
> >
> >  git-mergetool--lib.sh | 25 +
> >  mergetools/examdiff   | 18 ++
> >  mergetools/winmerge   | 21 +
> >  3 files changed, 44 insertions(+), 20 deletions(-)
> >  create mode 100644 mergetools/examdiff
> 
> This round looked good to me.
> David, does this look sensible to you?
> 
> Thanks.

Yes, thank you.

Acked-by: David Aguilar <dav...@gmail.com>

cheers,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Feature: custom guitool commands can now have custom keyboard shortcuts

[David Aguilar]

On Tue, Mar 29, 2016 at 11:29:41AM +, Harish K wrote:
> ---
>  git-gui/lib/tools.tcl | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)


I forgot to mention that git-gui has its own repository.
The git project merges the upstream repo as a subtree into its
git-gui directory.

You should probably clone the git-gui repository and create a
patch there so that it can be applied to the upstream repo:

http://repo.or.cz/w/git-gui.git
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Feature: custom guitool commands can now have custom keyboard shortcuts

[David Aguilar]

Hello,

On Tue, Mar 29, 2016 at 11:38:10AM +, Harish K wrote:
> ---
>  git-gui/lib/tools.tcl | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
> index 6ec9411..749bc67 100644
> --- a/git-gui/lib/tools.tcl
> +++ b/git-gui/lib/tools.tcl
> @@ -38,7 +38,7 @@ proc tools_create_item {parent args} {
>  }
>  
>  proc tools_populate_one {fullname} {
> - global tools_menubar tools_menutbl tools_id
> + global tools_menubar tools_menutbl tools_id repo_config
>  
>   if {![info exists tools_id]} {
>   set tools_id 0
> @@ -61,9 +61,19 @@ proc tools_populate_one {fullname} {
>   }
>   }
>  
> - tools_create_item $parent command \
> + if {[info exists repo_config(guitool.$fullname.accelerator)] && [info 
> exists repo_config(guitool.$fullname.accelerator-label)]} {
> + set accele_key $repo_config(guitool.$fullname.accelerator)
> + set accel_label 
> $repo_config(guitool.$fullname.accelerator-label)
> + tools_create_item $parent command \
>   -label [lindex $names end] \
> - -command [list tools_exec $fullname]
> + -command [list tools_exec $fullname] \
> + -accelerator $accel_label
> + bind . $accele_key [list tools_exec $fullname]
> + } else {
> + tools_create_item $parent command \
> + -label [lindex $names end] \
> + -command [list tools_exec $fullname]
> + }
>  }
>  
>  proc tools_exec {fullname} {
> 
> --
> https://github.com/git/git/pull/220

We also support "custom guitools" in git-cola using this same
mechanism.  If this gets accepted then we'll want to make
similar change there.

There's always a small risk that user-defined tools can conflict
with builtin shortcuts, but otherwise this seems like a pretty
nice feature.  Curious, what is the behavior in the event of a
conflict?  Do the builtins win?  IIRC, Qt handles this by
disabling the shortcut and warning that it's ambiguous.

Please documentation guitool..accellerator[-label] in
Documentation/config.txt otherwise users will not know that it
exists.

It would also be good for the docs to clarify what the
accelerators look like in case we need to munge them when making
it work in cola via Qt, which has its own mechanism for
associating actions with shortcuts.  Documented examples with
one and two modifier keys would be helpful.


cheers,
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html