`braid diff` silently ignores uncommitted changes

[mattmccutchen]

braid diff silently ignores uncommitted changes. It should at least give a warning or error, or better, include them in the diff.

(Going ahead and filing this, though I may be the one to implement it anyway.)

[mattmccutchen]

Some options for braid diff with uncommitted changes, none of them perfect:

1. Print an error and invite the user to run the command again with an --ignore-uncommitted option or so.
2. Print a warning. But the user might not notice the warning, especially if git diff invokes the pager and the warning is outside the paged output.
3. Set GIT_WORK_TREE to the mirror directory and run git diff against the upstream tree of the mirror. Ignores Attributes in the repository configuration that refer to the mirror path won't be applied correctly; e.g., if .git/info/attributes customizes the diff behavior for /skit1/my-notes.txt but we use skit1 as the root for the diff, then git will look up the file path of my-notes.txt and the attributes won't be used. [Edit: ignores don't affect git diff, but attributes may.] And unless we copy the relevant subtree of the real index file to a temporary index (which is more of a pain), git diff will lose the performance benefit of skipping files that are unchanged compared to the index, and the --cached option won't work.
4. Generate a base tree that has the upstream tree moved into the mirror directory (sort of like braid update does) and diff against that tree, limited to the mirror directory (so we don't need to load content outside the mirror directory like #42 changed braid update to do again). File paths in the diff output would include the mirror path; that's a change from the current behavior, though I personally don't find it any less reasonable than the current behavior. If the user passes their own path arguments to limit the diff, either we could require those arguments to already include the mirror path (e.g., braid diff skit1 skit1/layouts/layout.liquid) or we could prepend the mirror path automatically (e.g., braid diff skit1 layouts/layout.liquid -> git diff BASE_TREE skit1/layouts/layout.liquid).

I lean toward option 4. @realityforge, what do you think? Once we have a decision, I should be able to tackle #44 and #45 together.

[realityforge]

Ok I guess the way we typically use it is as follows. Make changes in project that may include changes in vendored directory. Maybe at some later point diff the changes in vendored directory an apply upstream and then braid update. i.e.

$ braid add https://github.com/user/mytool.git vendor/tool/mytool
$ vi foo.rb vendor/tool/mytool/bar.rb vendor/tool/mytool/baz.rb
$ git commit -a -m "..."
...
$ braid diff vendor/tool/mytool > ../mytool/patch.diff
$ cd ../mytool && git apply patch.diff && git commit -a - m "..." && git push && cd ../myproject
$ braid update vendor/tool/mytool

The ability to diff uncommitted could be useful in some circumstances as it would avoid the need to commit to the project before doing a braid update. So I am reluctant to change anything that would break the above process which from the sounds of it (4) would do by requiring users to pass some additional params to git apply to strip off the path prefix? However from my basic understanding (2) and (3) do not seem ideal and (1) seems like a cop-out. If somehow (4) could be adapted so that the existing paths come through that would be ideal.

As to the syntax braid diff skit1 skit1/layouts/layout.liquid versus braid diff skit1 layouts/layout.liquid - I don't really have a strong opinion. The first allows easy completion while the second form would require completion scripts if you wanted tab expansion. I can not remember at this stage if I have completion scripts already and am away from my development machine so can't check but if I do we could always add them to the project if the second form is desired.

[mattmccutchen]

I re-read the git diff man page and saw the --relative option, which will let us omit the mirror path from the file paths in the output in method (4). Regardless of this option, git diff will interpret its arguments relative to its working directory, so we can cd to get a particular interpretation or just keep the user's original working directory. I like the second option since it gives the user the flexibility to write the paths either way without needing a custom completion script, but it raises some issues:

1. None of the Braid commands correctly handles running in a subdirectory: they all look for .braids in the working directory. I'm willing to fix that.
2. Does it make sense to have the user to specify the full mirror path even when running Braid in a subdirectory? I'm leaning in favor.

Thoughts? Once we have the details decided, I can proceed to implementation.

Maybe at some later point diff the changes in vendored directory an apply upstream and then braid update.

I have some skepticism about whether we should encourage this workflow for sending changes back to the upstream project as opposed to something more automated that doesn't involve an intermediate patch, but for now it costs us nothing to continue supporting it. My use case for braid diff is to review the total change I've made to a mirror as I'm working, and it's definitely useful to run that without committing.

[realityforge]

--relative sounds like it would work and I concur that the second option sounds good. Making braid handle working in a subdirectory sounds like a good idea. Hmm which reminds me - one thing that really annoys some users is that .braids is not .braids.json as their editors do not work with it. Maybe I can get around to fixing that one soonish - I will start adding some issues to tracker to start to record some of these issues.

WRT to (2) Using a full mirror path means you can't use auto completion and it would probably make more sense to use relative paths but it would be a little more work to get it working. However I am easy if it does not end up this way.

As to whether the intermediate patch is required. I think there are probably some better alternatives. I have never used braid push but I expect that could be made to work, I could also see a new command like braid pull-request being useful. However I guess we have to account for the facet that the same people may not have access to the upstream projects as those who are working on the downstream projects. At least for us that is true and thus why braid push is unlikely to work. We typically sometimes require additional work before accepting the patch; sometimes minor tweaks, sometimes tests, sometimes testing to work in different environments (jruby versus mri ruby), different java versions etc. Anyhoo - something to keep in mind