stg refresh errors out when index contains added files and path limiting is given

[NonLogicalDev]

To reproduce:

( set -ex
  rm -rf test

  # Create Test Repo
  mkdir -p test && cd test
  git init && git commit -m "INIT" --allow-empty

  # Init StGit
  stg init

  # Create two new patches
  stg new patch1 -m patch1
  stg new patch2 -m patch2

  # Assert patch2 as topmost patch
  test "$(stg top)" == patch2

  # Touch a few files
  touch a.txt b.txt c.txt

  # Add all files to staging area (to make sure index is not dirty)
  stg add -A

  # Attempt to send only a.txt to patch1
  stg refresh -p patch1 -- a.txt

  ## ERROR: ##
  #
  # error: a.txt: cannot add to the index - missing --add option?
  # fatal: Unable to process path a.txt
  # stg refresh: git failed with code 128

  ## EXPECTED: ##

  stg files patch1
  ## OUTPUT: ##
  # A a.txt

  stg files patch2
  ## OUTPUT: ##
  # <EMPTY>

  stg status
  ## OUTPUT: ##
  # A b.txt
  # A c.txt
)

I narrowed down the problem to the following function:
https://github.com/stacked-git/stgit/blob/master/stgit/lib/git/iw.py#L332

[NonLogicalDev]

@jpgrayson I will brainstorm on how to fix this and wrap this in tests.
I tried simply adding the --add to the git update-index and that seemed to fix the error.

But there are more esoteric situations when this does not help.
Example of such is when a user has newly added files that ARE NOT in the index, and selects those using path limiting. Here changed_files does not notice untracked files, and the whole operation ends up a NOOP.

Ref: https://github.com/stacked-git/stgit/blob/master/stgit/lib/git/iw.py#L310

Illustration of my esoteric situation example:

( set -ex
  rm -rf test

  # Create Test Repo
  mkdir -p test && cd test
  git init && git commit -m "INIT" --allow-empty

  # Init StGit
  stg init

  # Create two new patches
  stg new patch1 -m patch1
  stg new patch2 -m patch2

  # Assert patch2 as topmost patch
  test "$(stg top)" == patch2

  # Touch a few files
  touch a.txt b.txt c.txt

  # Attempt to send only a.txt to patch1
  stg refresh -p patch1 -- a.txt


  stg files patch1
  ## EXPECTED: ##
  # A a.txt

  ## ACTUAL: ##
  # <EMPTY>

  stg files patch2
  ## OUTPUT: ##
  # <EMPTY>

  stg status
  ## EXPECTED: ##
  # ?? b.txt
  # ?? c.txt

  ## ACTUAL: ##
  # ?? a.txt
  # ?? b.txt
  # ?? c.txt
)

[jpgrayson]

Thank you for this thorough issue report @NonLogicalDev.

Adding --add to git update-index seems like a valid approach. At least I have not thought of any use cases where it would cause a problem.

Regarding the second "esoteric" scenario where you want to refresh with path limits specifying untracked files that should be added, I'm not sure that refresh should attempt to add those files. I.e. I think the current noop behavior is better than the proposed alternative. Consider this repo state:

 M subdir/tracked.txt
?? subdir/untracked.txt

I.e., we have a subdirectory with some tracked files and some untracked files. Then consider the following refresh command:

stg refresh -- subdir

With current StGit, the patch would only be refreshed with changes from the tracked file(s). This seems like sensible behavior.

But under the proposed semantics, I think refresh would both take changes from the tracked file(s) and add all untracked files from the subdirectory tree. This could lead to a pretty bad user experience if someone thought they were only refreshing the tracked files from the specified subdirectory.

We could potentially say that only files specified as path limits should be auto-added, but not directories. But that would be inconsistent with how path limits work in other contexts.

I'm inclined to take the conservative approach and require files to be explicitly added and not implicitly add files based on path limits.

[NonLogicalDev]

@jpgrayson I was thinking of that scenario, but you are bringing a good point.

Ideally we should make it very clear to the user that untracked files are not considered when path limiting is enabled.

It is also really weird assymetry.

• If you have added changed files and un-added changed files
  • stg barks at you for having a dirty index, path limiting stops working
• But if you have changed files (all added or all un-added) + untracked files
  • stg is happy and path limiting works

So if you want to path limit certain files in refresh you are better off adding everything to the index, and use path limiting.

Not saying this is a bad idea, but it is a substantial cognitive burden remembering this when working with Stg, which is fine for you and me who understand the ins and outs of how git index works, but might be a red flag for someone with a more surface level git understanding.

So I would suggest either erroring out, or warning the user / documenting this behavior in examples.

[jpgrayson]

Any changes you can think of that would help convey StGit's semantics to users would be appreciated.

So if you want to path limit certain files in refresh you are better off adding everything to the index, and use path limiting.

Or use stg add to add just the files (or hunks with git add --interactive) you want and then use stg refresh --index as an alternative to specifying path limits on the refresh command line. This is how I often deal with refreshing only a subset of changes in my working tree.

[NonLogicalDev]

@jpgrayson Likewise I feel like just adding files to the index is a more robust way of dealing with changes.
I am trying to get Stacked Git adopted at Uber, and some colleagues have been quite confused by this behavior.
Will brainstorm what would be a good way to call this out to get people to avoid this particular trap.