Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow difftool to be run outside of Git worktrees #163

Closed
wants to merge 3 commits into from

Conversation

dscho
Copy link
Member

@dscho dscho commented Mar 13, 2019

It was reported in git-for-windows#2123 that git difftool --no-index fails to work outside worktrees, even if it should work.

I fear this is a regression I introduced over two years ago (!) when I converted the Perl script to C.

At least now that I know about the bug, I can fix it.

Changes since v1:

  • Instead of ad-hoc parsing to look for --no-index, the OPT_ARGUMENT() was enhanced and now has its first real user! After all those lonely, long years (11 years, 1 month and 11 days), what a wonderful thing to celebrate on π day.
  • The test now uses the nongit helper.
  • The test uses test_cmp to make diagnosing regressions easier.

We will always spawn something from `git difftool`, so we will always
have to set `GIT_DIR` and `GIT_WORK_TREE`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Mar 13, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 13, 2019

Submitted as pull.163.git.gitgitgadget@gmail.com

`OPT_ARGUMENT()` is intended to keep the specified long option in `argv`
and not to do anything else.

However, it would make a lot of sense for the caller to know whether
this option was seen at all or not. For example, we want to teach `git
difftool` to work outside of any Git worktree, but only when
`--no-index` was specified.

Note: nothing in Git uses OPT_ARGUMENT(). Even worse, looking through
the commit history, one can easily see that nothing even
ever used it, apart from the regression test.

So not only do we make `OPT_ARGUMENT()` more useful, we are also about
to introduce its first real user!

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
As far as this developer can tell, the conversion from a Perl script to
a built-in caused the regression in the difftool that it no longer runs
outside of a Git worktree (with `--no-index`, of course).

It is a bit embarrassing that it took over two years after retiring the
Perl version to discover this regression, but at least we now know, and
can do something, about it.

This fixes git-for-windows#2123

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Mar 14, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 14, 2019

Submitted as pull.163.v2.git.gitgitgadget@gmail.com

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 12, 2019

This branch is now known as js/difftool-no-index.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 12, 2019

This patch series was integrated into pu via git@38cfaa8.

@gitgitgadget gitgitgadget bot added the pu label Apr 12, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Apr 16, 2019

This patch series was integrated into pu via git@e0b2eb4.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 16, 2019

This patch series was integrated into next via git@7313f9f.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 18, 2019

This patch series was integrated into pu via git@0e8e9f2.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 22, 2019

This patch series was integrated into pu via git@653b188.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 22, 2019

This patch series was integrated into pu via git@58bab6e.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 25, 2019

This patch series was integrated into pu via git@b72e907.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 25, 2019

This patch series was integrated into next via git@b72e907.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 25, 2019

This patch series was integrated into master via git@b72e907.

@gitgitgadget gitgitgadget bot added the master label Apr 25, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Apr 25, 2019

Closed via b72e907.

@gitgitgadget gitgitgadget bot closed this Apr 25, 2019
@dscho dscho deleted the difftool-no-index branch April 29, 2019 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant