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

feat: use $MERGED if present in environment, as the display path #734

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gthb
Copy link

@gthb gthb commented Jul 12, 2024

In #620 the display path was fixed to be based on the right-hand-side filename in the case that the left-hand-side filename is a git temporary file.

But that does not help for the case of git invocations comparing two git commits, such as:

  • git dft main...
  • git dft branch1...branch2

... because, in those cases, both the lhs and the rhs path are git temporary files, obscuring the real path to the file within the git repository.

The same applies if a file is removed or added, in which case one of the paths is a git temporary file and the other is /dev/null.

In those cases, git makes the real path available as the MERGED shell variable, for the purposes of constructing a difftool command line in git configuration.

So in Difftastic, let the real path be specified by the MERGED environment variable (overriding the lhs and rhs paths), and suggest export-ing that into Difftastic's environment in the git difftool configuration.

The resolution of Wilfred#620 does not help for the case of git invocations comparing
two git commits, like `git dft main...` or `git dft branch1...branch2`, because
in those cases both the lhs and the rhs path are temporary files, obscuring the
real path to the file within git.

In those cases, git makes the real path available as the MERGED shell variable,
for the purposes of constructing a difftool command line in git configuration.

So in Difftastic, let the real path be specified by an environment variable,
and suggest populating that with "$MERGED" in the git difftool configuration.
@Wilfred
Copy link
Owner

Wilfred commented Jul 20, 2024

Maybe difftastic should just respect MERGED when invoked from git, rather than expecting a REAL_PATH environment variable? Happy to merge this if you make that change :)

@gthb
Copy link
Author

gthb commented Jul 21, 2024

Maybe difftastic should just respect MERGED when invoked from git, rather than expecting a REAL_PATH environment variable? Happy to merge this if you make that change :)

That's what I tried first, hoping to not need any .gitconfig change ... but sadly the MERGED that git provides is just a shell variable, in the shell that invokes the difftool. So the difftool command line in .gitconfig must explicitly make it an environment variable.

I renamed it to MERGED just now, in c087c66, but it still needs to be there in .gitconfig:

 [difftool "difftastic"]
-        cmd = REAL_PATH="$MERGED" difft "$LOCAL" "$REMOTE"
+        cmd = MERGED="$MERGED" difft "$LOCAL" "$REMOTE"

That's why I had chosen the different name REAL_PATH, to avoid user confusion (“but I can just skip this, right?”).

The point is to make the shell variable an environment variable and this is the
more explicit way to do that. Thus less likely to cause user confusion about
whether/why this is necessary.
@gthb
Copy link
Author

gthb commented Jul 21, 2024

That's why I chose the different name REAL_PATH, to avoid user confusion (“but I can just skip this, right?”).

Just realized that this would be less confusing if the .gitconfig example explicitly uses export MERGED && so did that in 46e835b

@gthb

This comment was marked as outdated.

gthb added 2 commits July 21, 2024 22:13
I realized that this was using $MERGED in too specific a case, missing the
cases where a file was removed or added. Probably want $MERGED to be used as
the display filename in all cases, if it is present.
@gthb gthb changed the title feat: specify real path of file in environment feat: use $MERGED if present in environment, as the display path Jul 26, 2024
@gthb
Copy link
Author

gthb commented Jul 26, 2024

I noticed that this was missing the case where a file is removed or added, in the commit span being compared, so lhs or rhs is /dev/null. Moved this case out one level to cover that, in 53b899e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants