Skip to content

fix(git-bulk): fix workspace selection when cd fails #1197

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

Merged
merged 9 commits into from
Feb 26, 2025

Conversation

pierreay
Copy link
Contributor

cd may fails for multiple reasons:

  • mistake when editing .gitconfig manually
  • previously existing workspace that have been removed
  • ...

Currently, if cd fails, the BulkOp continue its execution ... in the workspace defined in a higher directory that where the user, despite the user specified a specific workspace (-w).

The user should be noticed of a failed cd (this is really not expected for a valid configuration) and the operations should stop to not execute something unexpected.

`cd` may fails for multiple reasons:
- mistake when editing `.gitconfig` manually
- previously existing workspace that have been removed
- ...

Currently, if `cd` fails, the `BulkOp` continue its execution ... in the
workspace defined in a higher directory that where the user, despite the
user specified a specific workspace (`-w`).

The user should be noticed of a failed `cd` (this is really not expected
for a valid configuration) and the operations should stop to not execute
something unexpected.
@pierreay pierreay marked this pull request as ready for review February 17, 2025 06:35
Copy link
Collaborator

@hyperupcall hyperupcall left a comment

Choose a reason for hiding this comment

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

Thanks!- Surprised this wasn't already here.

As a side-note, those other evals are a bit suspicious to me

Copy link
Collaborator

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Maybe we can also drop the cd in this PR?

Get rid of poor `eval` syntax because they are vulnerable to command
injection, which may have unexpected side effects.

However, they enabled a useful feature: using environment variable
(*e.g.*, defined in a `.bashrc`) inside the `.gitconfig` to use dynamic
paths as  `bulk` workspaces.

As such, I keep this feature possible by using the Bash's ${!VAR}
syntax, which allows to get the value of one variable using the name of
a another variable. However, arbitrary command injection is not possible
anymore.
@pierreay
Copy link
Contributor Author

Thanks!- Surprised this wasn't already here.

As a side-note, those other evals are a bit suspicious to me

I was surprised too, maybe not too many people invested time into git-bulk.
Anyway, in my last commit 7daedfd, I propose a patch which get rid of the evals while keeping the ability to use environment variables inside the .gitconfig (which is a feature I use, see my other PR#1191 for better context).

@pierreay
Copy link
Contributor Author

Maybe we can also drop the cd in this PR?

Which one? There is 3 of them ^^'. We cannot get rid of the 2nd one without changing guardedExecution which is designed to work in the current directory. The 3rd one do not seems neither useful nor harmful to me. The first one could be replaced by the path inside the find command and concatenated to the 2nd cd. But I don't see the pros of requiring to do so.

@spacewander
Copy link
Collaborator

Maybe we can also drop the cd in this PR?

Which one? There is 3 of them ^^'. We cannot get rid of the 2nd one without changing guardedExecution which is designed to work in the current directory. The 3rd one do not seems neither useful nor harmful to me. The first one could be replaced by the path inside the find command and concatenated to the 2nd cd. But I don't see the pros of requiring to do so.

Sorry for my typo! Actually, I want to drop the eval, which you have done in the new commit.

Copy link
Collaborator

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Should we doc the variable name feature added in this PR?

bin/git-bulk Outdated
if [[ ${rwsdir:0:1} == '$' ]]; then
# Dereference the `rwsdir` value which is a variable name.
rwsdir=${rwsdir:1}
rwsdir=${!rwsdir}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we exit 1 when $rwsdir is expanded to ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! Fixed in 145ecdd. :)

@pierreay
Copy link
Contributor Author

Should we doc the variable name feature added in this PR?

Documentation has been added in 4054cb3 and ebb6d37 for both Commands.md and man page. Note that in 0e4b6c4 I also mention the .gitconfig storing workspaces configuration, even if it is not strictly related to this PR.

@spacewander spacewander merged commit 8ce6300 into tj:main Feb 26, 2025
5 checks passed
@spacewander
Copy link
Collaborator

@pierreay
Merged. Thanks!

@pierreay pierreay deleted the fix/git-bulk-cd-failed branch February 27, 2025 15:33
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.

3 participants