I'm flying blind

Not a Windows user nor have access to a Windows box or VM

This is a plea for help!

On Windows, how to get the absolute path of these executables: sphobjinv and sphobjinv-textconv

These are incorrect

path_soi = Path(sys.executable).parent.joinpath("sphobjinv")
path_soi_textconv = Path(sys.executable).parent.joinpath("sphobjinv-textconv")

Affects

• tests/test_cli_textconv
• tests/test_cli_textconv_with_git

Packages site path: C:\Users\VssAdministrator\AppData\Roaming\Python\Python38\site-packages

site packages: ['c:\hostedtoolcache\windows\python\3.8.10\x64', 'c:\hostedtoolcache\windows\python\3.8.10\x64\lib\site-packages']

user site packages: 'C:\Users\VssAdministrator\AppData\Roaming\Python\Python38\site-packages'

path_scripts: C:\Users\VssAdministrator\AppData\Roaming\Python\Python38\SCRIPTS

I would say, take a look at the other CLI tests and see if it's possible to adapt the approach I took with them to your new tests.

It's possible the needs here are different enough that my existing technique is insufficient, but I'd at least give it a shot first.

(That is, if you haven't already.)

I'm not sure where the script is; looking at the output of this job step, though, it looks like it's in D:\a\1\s, at least for this run.

You could try the where command -- it's analogous to which on Linux.

I'm also not sure why you need to invoke the sphobjinv-textconv with the full path to the entrypoint script. It should be on path; can't you just call it without any path specified?

Will follow your advice and try with a relative path rather than an absolute path. You are right, the venv bin|SCRIPTS folder should already be on the path

I did some digging and testing -- a separate sphobjinv-textconv entrypoint that takes the single path argument isn't actually needed.

Combining this Stack Overflow answer with this section of the Git docs, if I add the following to my .gitattributes file:

*.inv diff=objects_inv

And add this to my .git/config:

[diff "objects_inv"]
	textconv = sh -c 'sphobjinv co plain "$0" -'

It works:

(sphobjinv) C:\git\sphobjinv>git diff
diff --git a/.gitattributes b/.gitattributes
index c6f20db..2a23375 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,2 +1,3 @@
 tests/resource/objects_mkdoc_zlib0.inv binary
 tests/resource/objects_attrs.txt binary
+*.inv diff=objects_inv




diff --git a/tests/resource/objects_textconv.inv b/tests/resource/objects_textconv.inv
index 31805f5..19a4323 100644
--- a/tests/resource/objects_textconv.inv
+++ b/tests/resource/objects_textconv.inv
@@ -2,6 +2,6 @@
 # Project: textconv-test
 # Version: 2.3
 # The remainder of this file is compressed using zlib.
-sphobjinv.cli.convert py:module 0 cli/implementation/convert.html#module-$ -
-sphobjinv.cli.convert.do_convert py:function 1 cli/implementation/convert.html#$ -
+sphobjinv.cli.convert23 py:module 0 cli/implementation/convert.html#module-$ -^M
+sphobjinv.cli.convert.do_convert234 py:function 1 cli/implementation/convert.html#$ -^M

So, while I really appreciate all of the effort you've gone to here, I believe you can already do what you want with the above recipe.

Open to correction, though, if I'm missing something.

Editing the file manually is easy, via code nontrivial.

• On Windows, sphobjinv executables path must be resolved

• On Windows, inventory path must be surrounded by single quotes so the backslashes are not removed by bash resulting in:
  C:hostedtoolcachewindowsPython3.8.10x64Scriptssphobjinv-textconv.EXE

• On Windows, double quotes does not do the backslash escaping. No idea, if "$0" would work as expected

git config diff.inv.textconv "sh -c 'sphobjinv co plain \"\$0\" -'"
git config diff.inv.textconv

sh -c 'sphobjinv co plain "$0" -'

Bash quote escaping is a nightmare, best avoided. UX wise, this is a nasty ugly non-obvious non-trivial solution requiring advanced bash skills and substantial hair loss (stressful) and code hidden in a config file.

End of the day, the end user needs smooth UX. These details at this level of complexity would have to be hidden.

I used my above-noted setup successfully without modification on both Windows (with Git for Windows) and Debian WSL, so it definitely works across those platforms.

What is the goal of editing the files via code? Providing the setup machinery so a user can run a command and the git textconv is set up automatically for them?

I'm confident (but still possibly wrong!) that the sphobjinv-textconv entry point, and thus also all the new testing around it, are not actually needed.

I like this capability a lot, but I don't want to add an 'automatically set up inv textconv' functionality to sphobjinv. It seems like it would be a bottomless source of support requests and edge case fixes.

It seems to me this is best handled by documenting and publicizing the method for setting it up. Different users will want it set up differently, and so the knowledge will be more valuable than automation here anyways.

1. A simple python solution is always better than a complicated bash solution. Even if you can, doesn't mean you should

2. UX wise, either method, but especially the bash method, needs a setup entrypoint. Could add that to soi-textconv. Didn't, to keep the PR scope limited. sphobjinv-textconv --setup. The bash route, recommend not adding to soi entrypoint. Already complicated.

So it's an UX issue. One command ... done. Much nicer than having to explain the intricacies of a complex sewer system.

What is the goal of editing the files via code?

Simplicity. Zero learning curve. Doing less work. Type one uncomplicated command ... done.

Seems like it would be a bottomless source of support requests and edge case fixes

Internally, soi-textconv runs git config. No one would have to manually edit the .git/config (nor the .gitattributes files) or care about the details.

Besides small changes to the parser, all the code for the setup has already been written and is known to work. Just refactor the integration unittest. After a refactor, the complexity of the integration test should be greatly reduced

The scope of soi-textconv is it configures git to understand inventory files. That scope shouldn't change besides the --setup and maybe --global flags.

@msftcangoblowm

TL;DR - You've done a lot of work here, but I'm afraid it's too much... more than I'm willing to add to the project. There's still an avenue to a smaller contribution, though.

I'm on board with:

• A new sphobjinv-textconv entrypoint to allow the git config entry to be cleaner
• A bit of basic documentation of how to set up the .inv textconv
  • Your current location for it in the docstring for the new cli/core_textconv.py makes sense for now, but I might change my mind
• A couple of targeted tests to exercise the new entrypoint.

But I don't want to uptake anything more than that:

• I would want the new entrypoint to be narrowly scoped.
  • It should take no options, and only the single INFILE argument.
  • The main sphobjinv entrypoint should remain the primary, and only configurable, CLI interface surface.
• For tests:
  • No direct testing of the operation of the git textconv.
  • Nothing new in the test suite driving git or editing .gitattributes or .git/config.
• For docs:
  • No tutorials on anything git other than brief instructions for setting up the textconv. It's just beyond the scope for the sphobjinv documentation.

So—if you're interested in paring down the PR to just the above (or, starting a new PR to only implement the above, which might be easier/faster), I think there's still a path to a merge here. Let me know if you want to move forward, and I'll leave more thorough comments/feedback on the PR.

I apologize that you went to all this work before getting this feedback, I didn't have any sense before you started that you were envisioning something of this scale. Lesson learned, I should've started a conversation about scale and approach before you started working!

All of this said, I'm still not convinced that the new entrypoint is actually required. Using user-scope .gitconfig and attributes files, and installing sphobjinv with pipx, I've been able to get the textconv working on both Git for Windows on Win11 and on Debian WSL without having a sphobjinv-containing virtualenv activated.

In $HOME/.gitconfig (WSL) & %USERPROFILE%/.gitconfig (Win11):

[diff "objects_inv"]
  textconv = sh -c 'sphobjinv co plain "$0" -'

In $HOME/.config/git/attributes (WSL) & %USERPROFILE%/.config/git/attributes (Win11):

*.inv diff=objects_inv

For WSL (similar but nonidentical on Win11):

$ sudo apt install -y pipx
$ pipx install sphobjinv

As we've already discussed, the benefit of sphobjinv-textconv would be a simpler invocation in the .gitconfig... but not that much simpler, given that either way, it's two canned snips of config for users to add into the right files.

It's really not much more than a quick post on a blog or Medium or wherever.

(I do understand that a magic command to insert those snips for users would make it even simpler for those users; but, that magic command would end up needing to accommodate a wide variety of operating systems and user setups, and I'm confident it would run into all sorts of hairy edge cases that would be a nightmare to debug and develop around. Far better IMO just to educate users on how to set it up, rather than trying to write something that will attempt to automatically set it up for a wide range of users.)

Regardless, again, if you're still interested in putting together the smaller-scope contribution for adding sphobjinv-textconv, I'm on board with working with you toward that.

The main sphobjinv entrypoint should remain the primary, and only configurable, CLI interface surface.

If you feel strongly sphobjinv --setup is better UX than sphobjinv-textconv --setup, i'm totally on board with whatever results in better UX.

This is actually a good idea that did not occur to me.

If the flag should have another name, just suggest a better flag name.

It should take no options, and only the single INFILE argument.

The --setup flag has to go somewhere. If it goes onto the sphobjinv CLI, then sphobjinv-textconv need not be changed any further.

A bit of basic documentation of how to set up the .inv textconv
No tutorials on anything git other than brief instructions for setting up the textconv. It's just beyond the scope for the sphobjinv
documentation.

On board with simple docs just saying for git diff support run optional command sphobjinv --setup. Then not explaining the manual method or the nitty gitty details.

No direct testing of the operation of the git textconv.
Nothing new in the test suite driving git or editing .gitattributes or .git/config.

Can move that out of fixtures and into code modules.

So—if you're interested in paring down the PR to just the above

Yes would be interested. Would like to change the current PR rather than making a new PR.

Please confirm i can go ahead.

but, that magic command would end up needing to accommodate a wide variety of operating systems and user setups, and I'm confident it would run into all sorts of hairy edge cases that would be a nightmare to debug and develop around.

When modifying .git/config, sphobjinv-textconv calls git config. So any hairy edge cases could only be caused by upstream. git config will have a flag controlling whether those changes affect local or globally.

We, the maintainers, need to have read and understood the git config manual. Specifically how to get/set.

When modifying .gitattributes, sphobjinv-textconv is doing that, but it's one line. The only edge case, is whether needs \r\n or not.

The setup changes are local; affecting one repo. Down the road, someone may request a local/global switch. Global setup goes into ~/.config/git/config and where ever the global .gitattributes is located.

There is a Python package specifically for platform xdg user and site folders. The platform specific xdg folders location will not an issue. I already have a wrapper module and unittest for this. So copy paste ... done

What I'm really hesitant about is whether I want to have a --setup (or whatever name) flag in sphobjinv at all. (On rereading what I wrote, I wasn't very clear about this -- apologies there.)

On one hand, I actually just used nbstripout seriously for the first time this week, which has a similar automated --install functionality, and it was really handy that it did it automatically for me.

On the other hand, these manipulations of git config and gitattributes really seem like something that should be encapsulated into their own library, and not rewritten in every library they're needed in. E.g., it seems like nbstripout has likely already implemented most or all of what would be required here (example)... and it would make way more sense to me, and I would be somewhat more inclined toward a sphobjinv-textconv --setup feature, if the git config/attributes manipulations were pulled into a library that --setup could call, instead of being re-implemented directly in the sphobjinv codebase. (Plus, from even a quick scan of the nbstripout code, I noticed multiple things that are sharp edges just waiting for users to eventually cut their fingers on... it's hard to actually do this the right way.)

Down the road, someone may request a local/global switch.

Yeah... and I could see at least the following sorts of requests popping up, too:

• Can you make it install system-wide, not global or per-repo?
• Can you make it tell me where the textconv is installed?
• Can you make it uninstall the textconv for me?
• I have a complicated setup, and your --setup broke my git. Can you undo what you did?
• Can you make it install other gitconfig/gitattributes settings?
• Can you expose the --setup functionality in the API?

Just a really large possible CLI/API surface that might be requested.

Ultimately, this sort of git config/attributes manipulation is solidly outside sphobjinv's mission, and I really don't want to open the door to issues/features surrounding the config/attributes manipulations themselves being on-topic for its issue tracker.

I'm open to continued discussion, but for now I'm only willing to consider the sphobjinv-textconv entrypoint itself, as an alias for sphobjinv convert plain $0 -, and documentation of the manual process for setting up the textconv.

You are right.

This should be a separate package, named git-filter, for interacting with git config and gitattributes.

It's not a sphobjinv specific issue. sphobjinv users could benefit from such a package.

git-filter install --local --package=sphobjinv

git-filter remove --local --package=sphobjinv

git-filter list --package=nbsplitout

And yaml files describing changes need by each package. Was thinking plugins, but that is overkill

Thanks again for the work you did on this, @msftcangoblowm -- sorry it didn't end up being merged.

FYI, I did a bit of searching for other projects that have attempted this sort of git config, and found one: https://github.com/d12frosted/git-config-manager. I'm not sure what language it was written in, but it might be a useful resource.