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

git-gui: fix GIT_DIR handling for submodules. #2452

Merged
merged 2 commits into from
Jan 3, 2020
Merged

git-gui: fix GIT_DIR handling for submodules. #2452

merged 2 commits into from
Jan 3, 2020

Conversation

oldium
Copy link

@oldium oldium commented Jan 3, 2020

When dealing with submodule, the GIT_DIR might not be set. If it is not set
before, it cannot be unset and the execution ends with error:

can't unset "env(GIT_DIR)": no such element in array

Fix this by checking if GIT_DIR can be unset.

@dscho
Copy link
Member

dscho commented Jan 3, 2020

This PR looks good to me, but in the meantime Git GUI has an active maintainer again, so may I ask you to rebase this and open a GitGitGadget PR? I.e.

  1. git remote add gitgitgadget https://github.com/gitgitgadget/git
  2. git fetch gitgitgadget git-gui/master
  3. git branch --set-upstream-to=gitgitgadget/git-gui/master
  4. git rebase --onto @{u} HEAD^
  5. force-push
  6. open a PR at https://github.com/gitgitgadget/git
  7. ping me (or other active people) in the PR to get permission to use GitGitGadget
  8. add a single comment /submit to send the patch off to the Git mailing list, Cc:ing the Git GUI maintainer

@dscho
Copy link
Member

dscho commented Jan 3, 2020

Git for Windows has different code for Git GUI, so this fix is specifically for Git for Windows. The main difference is commit ca2f932, which is not present in mainline Git

Oh, that's a good point!

(and I actually do not understand why it is there).

Right. I did stumble over it a couple times, and once I tried to figure out what was the rationale, but also failed.

Maybe we should just revert the revert instead, i.e. drop it from Git for Windows' patches?

@oldium
Copy link
Author

oldium commented Jan 3, 2020

Yes, drop ca2f932 together with a9af64e.

@dscho
Copy link
Member

dscho commented Jan 3, 2020

Yes, drop ca2f932 together with a9af64e.

I found the reason for ca2f932! Look at https://groups.google.com/d/msg/msysgit/FkdTopndAes/RXF_h8eAgW4J

@dscho
Copy link
Member

dscho commented Jan 3, 2020

Apparently, this failed (in a worktree with an initialized submodule):

git gui
# the following instructions are for clicking

  • Tools->Add...
    Enter Name: submodule update
    Enter Command: git submodule update
  • Add
  • Tools->submodule update

Error message is:
"fatal: reference is not a tree: 543f8d6
"Unable to checkout '543f8d61eb235820aecefde6e46ec90e9803a6d8' in submodule path 'doc/git/html'"

@oldium you probably have a worktree with initialized submodule(s), right? Could you try whether the indicated bug is still there after reverting a9af64e and ca2f932?

@oldium
Copy link
Author

oldium commented Jan 3, 2020

Yes, I do. I will play with this a little bit.

@dscho
Copy link
Member

dscho commented Jan 3, 2020

@oldium thank you.

I dug a little further, and it seems that https://groups.google.com/d/msg/msysgit/FkdTopndAes/BRVBc95idk8J was intended to (quote:) "fix this properly", and that patch made it into Git's master via the merge bd08ecc (the patch in question is 74ae141).

So yes, it should be possible for us to just drop those two commits.

If your testing confirms this, would you terribly mind updating this here PR accordingly? It is unfortunately too late for v2.25.0-rc1 (which is already in the process of being released), but it will make it into -rc2 at least.

@oldium
Copy link
Author

oldium commented Jan 3, 2020

I tried the following which I found in the mailing list:

git clone git://repo.or.cz/msysgit.git
cd msysgit
git submodule update --init
git gui
# the following instructions are for clicking
* Tools->Add...
Enter Name: submodule update
Enter Command: git submodule update
* Add
* Tools->submodule update

Result:
image

@dscho
Copy link
Member

dscho commented Jan 3, 2020

Perfect! So we can drop this revert after slightly less than 10 years ;-)

oldium added 2 commits January 3, 2020 22:40
This reverts commit a9af64e.

Signed-off-by: Oldřich Jedlička <oldium.pro@gmail.com>
This reverts commit ca2f932.

Signed-off-by: Oldřich Jedlička <oldium.pro@gmail.com>
@oldium
Copy link
Author

oldium commented Jan 3, 2020

Updated PR.

@dscho dscho merged commit 7920803 into git-for-windows:master Jan 3, 2020
@dscho dscho added this to the v2.25.0 milestone Jan 3, 2020
dscho added a commit to git-for-windows/build-extra that referenced this pull request Jan 3, 2020
Git GUI [can now deal with uninitialized
submodules](git-for-windows/git#2452) (this
was a Windows-specific bug).

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

dscho commented Jan 3, 2020

Thank you @oldium!

@oldium oldium deleted the fix-unset-env branch January 3, 2020 22:44
@oldium
Copy link
Author

oldium commented Jan 4, 2020

You are welcome, @dscho!

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