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

Enable vcsh ls-file to take an option '-p' #285

Merged
merged 5 commits into from
Apr 3, 2021

Conversation

alerque
Copy link
Collaborator

@alerque alerque commented Apr 2, 2021

Closes #247.

This is a re-submission of PR #252 which was closed automatically by the GitHub overloads when we renamed the default branch. Since the downstream fork no longer existed it closed rather than reparented some PRs, this being one of them.

Per my comments on the related issue I'm not sure this is the right approach, but if we do further work in this direction we can at least build on and attribute the commit.

@alerque alerque mentioned this pull request Apr 3, 2021
@RichiH
Copy link
Owner

RichiH commented Apr 3, 2021

What heuristic should we aim for with spacing? Currently this turns

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   ../../../../../.zshrc

into

zshrc: Changes not staged for commit:
zshrc:   (use "git add <file>..." to update what will be committed)
zshrc:   (use "git restore <file>..." to discard changes in working directory)
zshrc: 	modified:   ../../../../../.zshrc

i.e. the removed spacing in front is different.

There seem to be two options:

  1. Remove all whitespace in front and then add a single whitespace or tab after repo:
  2. Keep the whitespace completely intact, but that clashes with tabs.

@alerque
Copy link
Collaborator Author

alerque commented Apr 3, 2021

My preference would be to not trim any space at all and always add repo_name:<space> prefix, then have byte for byte full lines after that. This means we have to adjust the way read() works so it doesn't trim anything...or use something suited to the task.

vcsh Outdated
GIT_DIR=$VCSH_REPO_D/$VCSH_REPO_NAME.git; export GIT_DIR
use
$command_prefix "$@"
$command_prefix "$@" | eval $command_suffix
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't feel right to me, I think we can probably do better with an exec call before running the command to setup the output stream processing. That would avoid piping everything through cat even when this option is not active, hence not potentially breaking existing possibilities.

vcsh Outdated
if [ x"$1" = x'-g' ]; then
unset command_prefix
fi
if [ x"$1" = x'-p' ]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method of option parsing just doesn't work right at all, you can't use -g -p or -p -g because the manual shift is eating the argument and throwing getopts() off. Speaking of which, this isn't how getopts() is supposed to work at all, and the flag variable it sets is being ignored. I don't know how the top level option parsing got away with this for so long, but I don't think it's correct either....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See also #286 for a PR that fixes the getopts() usage for the main script.

@alerque alerque force-pushed the github-issue-247 branch from 6e488c8 to 87c12fe Compare April 3, 2021 09:26
@alerque
Copy link
Collaborator Author

alerque commented Apr 3, 2021

I think I'm okay with this now. It's not quite as elegant as I would have liked, but it should be robust and the repetition is just because POSIX is more limiting than the ZSH scripting I'm used to.

Note @RichiH whichever order this and #286 are merged the second one will need a merge conflict resolution. I already have it resolved both ways in git rerere and am happy to do the merges, but I'm leaving the PRs for you to at least review first. Merge conflict fixed.

@alerque alerque marked this pull request as ready for review April 3, 2021 09:44
@alerque alerque requested a review from RichiH April 3, 2021 09:44
@alerque alerque marked this pull request as draft April 3, 2021 13:56
@alerque alerque marked this pull request as ready for review April 3, 2021 13:59
@RichiH
Copy link
Owner

RichiH commented Apr 3, 2021

I still hate the tab in front of modified in e.g. #285 (comment) as it changes how the output looks; but I am also not sure if it's on us to clean that up.

getops long is a pain with POSIX, else I would prefer a long-opt. I wonder if it makes sense to keep -p clear and use -P as it seems a nice use case, but I am undecided either way. Thoughts?

@alerque alerque marked this pull request as draft April 3, 2021 14:36
@alerque
Copy link
Collaborator Author

alerque commented Apr 3, 2021

I still hate the tab in front of modified in e.g. #285 (comment) as it changes how the output looks; but I am also not sure if it's on us to clean that up.

I don't like it either, but that's on Git not us. If we start down the road trying to sanitize output from other commands according to the way we want to see it we'll never come out alive. This way (prefixing the exact full line) is reversible, the user could choose to parse it (say with awk) and easily strip out the field and have an exact replica of what they would have gotten manually without VCSH in the middle.

getops long is a pain with POSIX, else I would prefer a long-opt. I wonder if it makes sense to keep -p clear and use -P as it seems a nice use case, but I am undecided either way. Thoughts?

Yes, we have no good solution for long opts or I would go that route too. I can see wanting to keep -p clear and going with -P.

Here's the deal though, I just realized this getopts() loop can get clobbered by the upper one. Least ways using upper ones like -v are killing the lower one's ability to parse anything. I'll look into that.

@alerque
Copy link
Collaborator Author

alerque commented Apr 3, 2021

This PR isn't any more broken than it already was. In order to use the existing -g flag you cannot use the main app's -v or other flags. I suspect the same is true of other subcommands that have flags as well.

I suggest we go ahead and put this through knowing it only works without any top level flags and work on fixing that snafu separately.

As such I can't think of any good reason not to use lower case -p. Either case has a potential misinterpretation of Git's respective arguments, but the only way we are going to get around that is properly using an argument guard (which also needs to be done at the top level, and again should probably be evaluated in a different PR.

@alerque alerque marked this pull request as ready for review April 3, 2021 14:59
@RichiH
Copy link
Owner

RichiH commented Apr 3, 2021

I tested this for #286 and it works, though:

richih@roadwarrior3 (git)-[remotes/alerque/fix-getopts] ~git/richih/vcsh % git log | head -n1
commit 78a05e1ad340d03cdc55bd5f57089b84779ee513
richih@roadwarrior3 (git)-[remotes/alerque/fix-getopts] ~git/richih/vcsh % ./vcsh -v foreach -g echo foo
verbose mode on
vcsh 1.20141026
vcsh: verbose: foreach begin
zshrc:
foo
vcsh: verbose: foreach end, exiting
richih@roadwarrior3 (git)-[remotes/alerque/fix-getopts] ~git/richih/vcsh % 

(this reminded me of how brittle my "what version are you" system is, and means I will need to fix it for the safe harbor release. Good thing I currently have a release branch as main is already clobbered. Decent argument in favor of an actual build system)

@alerque
Copy link
Collaborator Author

alerque commented Apr 3, 2021

For now I sugges tagging a new safe harbor release as a new commit with the current one as its parent, then we'll merge that back into main so it's in the history (and git describe works). I'm experimenting with a configure/build system on the side and should have something to show soon to avoid this in the future.

@alerque
Copy link
Collaborator Author

alerque commented Apr 3, 2021

Amending my previous comment, I actually don't think you need to do anything with the safe harbor release at all. That was a freeze frame as Git HEAD existed and identified itself not a full release cycle as might otherwise have been. I don't think the fact that it identifies itself the same way cb33546 did is really that much of a problem. It's more obnoxious to me that the current HEAD builds still identify the same way and hence its useless for debugging, but I'll fix that soon enough with some build tooling.

If you do add a tag, lets make it a commit branched from dbc9c07 that fixes VERSION in the script, then tag it v1.20190619.1. Then we can merge that back into master without too much fuss. If you want me to make it happen I can. But again I think it's okay to just leave it and work towards a v2.0.0.

@alerque
Copy link
Collaborator Author

alerque commented Apr 3, 2021

I tested this for #286 and it works, though:

richih@roadwarrior3 (git)-[remotes/alerque/fix-getopts] ~git/richih/vcsh % git log | head -n1
commit 78a05e1ad340d03cdc55bd5f57089b84779ee513
richih@roadwarrior3 (git)-[remotes/alerque/fix-getopts] ~git/richih/vcsh % ./vcsh -v foreach -g echo foo
verbose mode on
vcsh 1.20141026
vcsh: verbose: foreach begin
zshrc:
foo
vcsh: verbose: foreach end, exiting
richih@roadwarrior3 (git)-[remotes/alerque/fix-getopts] ~git/richih/vcsh % 

I don't understand how you are getting that result. Here is what I get:

$ git checkout 78a05e1a
HEAD is now at 78a05e1 Fix broken getopts() usage, allows multiple flags
$ git reset --hard
HEAD is now at 78a05e1 Fix broken getopts() usage, allows multiple flags
$ git clean -dxff
$ ./vcsh -v foreach -g echo foo
verbose mode on
vcsh 1.20141026
vcsh: verbose: foreach begin
que-atom:
git: 'echo' is not a git command. See 'git --help'.

The most similar command is
        fetch
que-awesome:
git: 'echo' is not a git command. See 'git --help'.

The most similar command is
		fetch
...

And so on with the same output for all of my repos.

Is it possible that your Git is behaving differently than mine and passing unknown subcommands to the shell? Or that you have a git-echo installed somewhere? Or do we have different getopt() implementations? My /bin/sh is actually Bash's POSIX compatibility mode "GNU bash, version 5.1.4". What is yours?

@alerque
Copy link
Collaborator Author

alerque commented Apr 3, 2021

Oh lovely. I just dried using dash instead of my system's default sh symlink that invokes bash with compatibility mode on — and it works. So this is going to be fun to sort out.

$ dash vcsh -v foreach -g echo foo
verbose mode on
vcsh 1.20141026
vcsh: verbose: foreach begin
que-atom:
foo
que-awesome:
foo
...

@alerque
Copy link
Collaborator Author

alerque commented Apr 3, 2021

I think I'm going to go ahead and merge this. Fixing shell compatibility in option parsing is out of scope and this PR doesn't make it worse.

@alerque alerque merged commit 6da3173 into RichiH:main Apr 3, 2021
@alerque alerque deleted the github-issue-247 branch April 3, 2021 18:40
@alerque alerque added this to the v2.0.0 milestone Jun 23, 2021
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.

foreach greppable mode
3 participants