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

Toggle execute permission #6135

Closed
wants to merge 8 commits into from

Conversation

tip2tail
Copy link

Proposed fix for #3176 feature request.

First time contribution to this project. Happy to refine this subject to review/advice. Not convinced the wording for the dialog messages is perfect so any suggestions from maintainers will be very welcome for this.

I use GitExtensions every day, on projects which are released to Linux servers and I regularly need to set the executable flag on shell scripts etc. This functionality will save me quite a bit of time.

Proposed changes

  • Add new menu item to toggle the unix executable file permission flag
  • Add new flag in GitItemStatus class to indicate if file is executable
  • New methods added to GitModule

Screenshots

Before

before

After

after

Test methodology

  • Tested on various projects
  • Ensured permission was set correctly
  • Successfully executed scripts which had been set to executable using new option

Test environment(s)

  • GIT 2.20.1.windows.1
  • Windows 10 (1809 - OS 18309.10000)

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RussKie will request tests too

GitUI/CommandsDialogs/RevisionFileTreeControl.Designer.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/RevisionFileTreeControl.Designer.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/RevisionFileTreeControl.cs Outdated Show resolved Hide resolved
@tip2tail
Copy link
Author

tip2tail commented Jan 20, 2019

RussKie will request tests too

Thanks @gerhardol - I can't see a file for tests for this menu. Having never created tests in this manner before I'm unsure where to begin - any guidance welcome on this!

@codecov
Copy link

codecov bot commented Jan 20, 2019

Codecov Report

Merging #6135 into master will decrease coverage by 0.03%.
The diff coverage is 7.69%.

@@            Coverage Diff            @@
##           master   #6135      +/-   ##
=========================================
- Coverage   45.23%   45.2%   -0.04%     
=========================================
  Files         657     657              
  Lines       49607   49659      +52     
  Branches     6586    6607      +21     
=========================================
+ Hits        22442   22448       +6     
- Misses      25910   25958      +48     
+ Partials     1255    1253       -2

@gerhardol
Copy link
Member

There are some tests in UnitTests\GitUITests\CommandsDialogs\RevisionFileTreeControllerTests.cs

@tip2tail
Copy link
Author

There are some tests in UnitTests\GitUITests\CommandsDialogs\RevisionFileTreeControllerTests.cs

Thanks @gerhardol, although I still am unsure of how/what I can add in this file to test the new context menu item or the underlying routines. I don't see any tests for the existing context menu items I am adding this near so I have nothing to base my new code on in order to experiment/learn.

@RussKie
Copy link
Member

RussKie commented Jan 20, 2019 via email

@tip2tail
Copy link
Author

It is important to remember that the file tree isn't showing the content of the working directory, but what was committed in a given commit. So the proposal is only constrained to a very specifc edge case. I have not looked at the proposed implementation, but it must be Linux specific and the menu must be shown only for files that exist in the working directory.

Thanks @RussKie & @gerhardol. I will refine this further today and update this PR.

Ref your comment about "Linux specific", if you mean the menu option should only be available if GitExtensions is running on Linux then I disagree. In Windows, I work daily on PHP projects that are subsequently released to a Linux server for production. These projects often contain *.sh bash script files which need to be executable when the repo is deployed on Linux. That is the use-case for this and for that reason I believe this option should be available regardless of running platform.

@RussKie
Copy link
Member

RussKie commented Jan 20, 2019 via email

@tip2tail
Copy link
Author

tip2tail commented Jan 20, 2019

@RussKie sure.

Taking one project as an example - all are similar:
I have a website that is PHP based. The repo is cloned on the production server and the "production" branch is checked out. This is the live site.

In the repo are some shell scripts (e.g. cron.sh which carries out various tasks on the Linux crontab). These files are written on Windows but if I simply add and commit any new scripts to my repo with GitExtensions then they would not be executable on the server.

Running chmod on the files on the server would then cause a change to the repo at that side, resulting in a messy (and un-automated) process to deploy future updates as these are done via git pull.

The work around currently is to run git update-index --chmod=+x cron.sh from my Git bash session in the repo on Windows and then commit using GitExtensions. That way the files are automatically executable when the repo is updated on the server.

My PR is intended to provide a menu option to prevent having to go to git bash each time. Noticing that this feature request was open would suggest I am not the only user with a similar need.

Hope that helps you understand my thinking :)

@RussKie
Copy link
Member

RussKie commented Jan 20, 2019

Thank you.
I propose to tackle the problem in a slightly different but more generic manner.

GE has a notion of "user scripts", which a user can write and execute as necessary.
IIUIC, currently user scripts are only for revisions (i.e. right click on a revision in the revision graph and chose to execute a script).
image

We could extend scripts to appear in the "File tree" panel as well.
image

This way a user could have an arbitrary number of different scripts, per file extension, per folder, per repo etc.
For example I badly want a script to start VS solution or open folder in VSCode, but I can't do that from GE. With the above proposal, I would be able to do what I desire.

What do you think?

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is also applicable for Cygwin
The functionality is comparable to Edit working directory file

GitUI/CommandsDialogs/RevisionFileTreeControl.Designer.cs Outdated Show resolved Hide resolved
@tip2tail
Copy link
Author

Hi @RussKie .

I do think that your proposal is a good idea, i.e. to implement the "scripts" on files. However, I also think that the menu option I propose should be included. It would prevent a user from having to create a script in the first place - I didn't know about the functionality, I would suspect others wouldn't either, and for smaller simple projects I would think it is overkill to create a script etc. IMO, if that was the "solution" I would likely continue to do the actions I describe above in git bash.

@gerhardol - thanks, I had forgot about Cygwin on Windows. That makes all the more reason to include the menu option IMO. I will change that wording again :)

@vbjay
Copy link
Contributor

vbjay commented Jan 20, 2019

If we are going to extend scripts, I have an idea about that.

Have a script visibility option to show the script or not. We could use linq to filter scripts so that if you right click a file in the unstaged area you get scripts that match All | Unstaged. Will need to apply this to the script variables too.

  • Revision grid
  • Empty Revision Grid
  • file lists ( diff, file tree...)
  • unstaged files in commit dialog
  • staged files in commit dialog
  • ...
  • All ( show every where)

A great example. Could improve this to create a duplicate script like it but add selected files as a parameter to only stage the files that are selected. The menu could show on file lists and unstaged file list and the revision grid without the file parameter.

@tip2tail
Copy link
Author

Hi @vbjay

I too agree with extending scripts etc. but would that not be a separate issue rather than a solution to this feature request?

Thanks

@vbjay
Copy link
Contributor

vbjay commented Jan 21, 2019

Of course. The idea was expressed. I tossed in my thoughts on it. Although, Russkie has a good point. This really should be a user script and not coded in the UI.

@RussKie
Copy link
Member

RussKie commented Jan 21, 2019 via email

@tip2tail
Copy link
Author

I understand, but I do feel the use-case is sufficient that a simple menu option would make more sense.

See the following for others having the same problems in "Sourcetree":
https://stackoverflow.com/questions/49159181/in-sourcetree-for-windows-how-to-chmod-set-executable-permission-bit

@vbjay
Copy link
Contributor

vbjay commented Jan 21, 2019 via email

@RussKie
Copy link
Member

RussKie commented Jan 22, 2019

GE scripts perhaps isn't widely known and certainly deserve more polish and limelight.
You can find them under Settings:
image

Right now the scripts are scoped to git commands events and the revision graph.
I think you may find that a script is significantly better suited for the issue you're having right now. E.g. you could add a script that automatically executes chmod +x *.sh once you have checked out a revision (event AfterCheckout).

@sharwell sharwell added the 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed label Jan 23, 2019
@tip2tail
Copy link
Author

File updated as requested @sharwell :)

@sharwell sharwell added 🖊️ status: cla signed and removed 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed labels Jan 26, 2019
@RussKie
Copy link
Member

RussKie commented Jan 27, 2019

Have you had a chance to try out scripts?

@tip2tail
Copy link
Author

tip2tail commented Feb 5, 2019

Hi @RussKie

I haveent trid out scripts. I will give them a look.

It still doesnt change my thoughts that as this is a feture of Git that it should be included in the interface proper. The code is here now, but obviously I leave it to the maintainers to decide whether to accept this small change.

Thanks

@tip2tail
Copy link
Author

tip2tail commented Feb 5, 2019

@illfated thanks for the heads up. I have now done that :)

@RussKie
Copy link
Member

RussKie commented Feb 10, 2019

Thank you for the links.
However they have further reinforced my belief that the issue is better addressed via a GE script, e.g. the following can be run after every checkout https://stackoverflow.com/a/32717779/2338036
The script can be baked in to GE and (possibly?) enabled by default.

The problem with the context menu in the file tree on a random revision is that revision is a snapshot of what the repository looked like at the time of a commit, and not (necessarily) what it is now.
Thinking otherwise is wrong, and thus adding an action that is only relevant to the working tree is incorrect.

@RussKie RussKie added 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed and removed 🖊️ status: cla signed labels Feb 15, 2019
@NOYB
Copy link

NOYB commented Feb 24, 2019

As the issue 3176 originator I agree mostly with tip2tail that this feature should be exposed to the user via a menu option. Perhaps adding the option to context sensitive menus such as when right clicking on a file in the; commit window, tree view, diff view, etc. A lot of us don't need to automate this. We just need to make a file executable in a repo here and there. I have to go find my cheat sheet, open up the bash shell, find the dir/file, enter the command. Way more work that it should be.

Should scripts be expanded? Probably. But as tip2tail has pointed out. That is a different matter. This feature should be implemented independent of scripts expansion.

@vbjay
Copy link
Contributor

vbjay commented Feb 24, 2019 via email

@ghost
Copy link

ghost commented Feb 24, 2019

"This branch has conflicts that must be resolved"
(another rebase needed to resolve the conflict)

@tip2tail
Copy link
Author

tip2tail commented Mar 3, 2019

Re: @vbjay "The scripts would create the menu."

If that is the case then why not just have the menu rather than making users use a lesser known "feature" to have to go and create/enable a script that ends up doing the sames thing.

I kind of get what @RussKie is driving at regarding the tree being a snapshot of the directory at that commit, however as others have pointed out I'm pretty sure there are other options in that menu that act on the curent working directory as well - so its not really a difference in functionality there. Precaustions have been made in terms of the message to the user and the enabled status of the menu item itslef that this should be sufficiently clear.

Thanks @NOYB for the support.

If there is genuinely no appetite for this feature among the maintainers of the project then please let me know and I will close and move on. However I really do hope you can consider what I feel would be a helpfull addition to a great tool.

If this is to proceed I will of course rebase again - thanks @illfated for pointing that out :)

@gerhardol
Copy link
Member

It is @RussKie that does the majority of the work and has the vision - he has the majority of "votes".
GE has a number of "exotic" features already, sometimes the maintainer has to steer the direction.
I do allow the functionality, but the menu text should be point out that this is for the working directory.
A script is an option.

@RussKie RussKie added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed status: more info requested labels Sep 19, 2019
@ghost ghost added the 💤 status: no recent activity The issue is stale label Oct 19, 2019
@ghost
Copy link

ghost commented Oct 19, 2019

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs.

@GintasS
Copy link
Collaborator

GintasS commented Aug 6, 2021

This is an old PR and should be closed or set to a draft. @RussKie

@ghost ghost removed the 💤 status: no recent activity The issue is stale label Aug 6, 2021
@ghost ghost added the 💤 status: no recent activity The issue is stale label Sep 8, 2021
@ghost
Copy link

ghost commented Sep 8, 2021

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👓 status: needs review 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed status: abandoned type: discussion 💤 status: no recent activity The issue is stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants