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

Stage selected lines broken on master #730

Closed
lucaotta opened this Issue Oct 5, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@lucaotta

lucaotta commented Oct 5, 2017

It looks like that stage selected lines is broken on master branch, as well as on version 2.11 as released on homebrew.

I'm trying to stage a single hunk, and I get an error while applying the patch, in cmds.py:235

status, out, err = self.model.apply_diff_to_worktree(tmp_file)

The error returned is:

u'error: patch failed: C++/Support/FactoryTest/factory_test.cpp:9\nerror: C++/Support/FactoryTest/factory_test.cpp: patch does not apply\n'

Backtrace:

  /Users/luca/src/electrolux/parrot/git-cola(56)<module>()
  /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/main.py(31)main()
-> return args.func(args)
  /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/main.py(291)cmd_cola()
-> return application_start(context, view)
  /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/app.py(357)application_start()
-> result = context.app.exec_()
  /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/app.py(242)exec_()
-> return self._app.exec_()
  /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/qtutils.py(39)<lambda>()
-> action.triggered[bool].connect(lambda x: fn())
  /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/widgets/diff.py(582)apply_selection()
-> self.process_diff_selection()
  /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/widgets/diff.py(610)process_diff_selection()
-> self.has_selection(), reverse, apply_to_worktree)
  /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/cmds.py(1905)do()
-> return do_cmd(cls(*args, **opts))
  /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/cmds.py(1912)do_cmd()
-> return cmd.do()
> /Users/luca/Applications/git-cola.app/Contents/Resources/share/git-cola/lib/cola/cmds.py(239)do()
-> core.unlink(tmp_file)

I've verified that I can stage the same hunk using git gui.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Oct 6, 2017

Member

Would it be possible to reproduce this using a recipe or public repo? There's probably something particular about the specific diff that needs to be handled (for example, the diff's location within the file affects the diff header line).

There's a temp file that cola writes that could be useful to inspect as well. The line where it does core.unlink(tmp_file) could be replaced with print(tmp_file). We can inspect the printed path to see the diff it's producing.

The diff writer probably has an edge case it needs to handle. I use stage selected all the time and it's been working reliably (for me) so there could be a usage pattern I'm not hitting.

Member

davvid commented Oct 6, 2017

Would it be possible to reproduce this using a recipe or public repo? There's probably something particular about the specific diff that needs to be handled (for example, the diff's location within the file affects the diff header line).

There's a temp file that cola writes that could be useful to inspect as well. The line where it does core.unlink(tmp_file) could be replaced with print(tmp_file). We can inspect the printed path to see the diff it's producing.

The diff writer probably has an edge case it needs to handle. I use stage selected all the time and it's been working reliably (for me) so there could be a usage pattern I'm not hitting.

@lucaotta

This comment has been minimized.

Show comment
Hide comment
@lucaotta

lucaotta Oct 6, 2017

Unfortunately the repo is not public. I've verified that this happens only on one particular file, for other files the functionality is working well.
The patch file produced is the following:

--- a/C++/Support/FactoryTest/factory_test.cpp
+++ b/C++/Support/FactoryTest/factory_test.cpp
@@ -9,6 +9,7 @@ PowerupStateMachine::PowerupStateMachine(AlarmMng *alarms, bool fctTestDone, boo
     if ( !fctTestDone )
         m_state = FactoryStep1;
     m_alarms = alarms;
+    m_state = Normal;
 }
 
 PowerupStateMachine::States PowerupStateMachine::state() const

I'm on Mac, I launch git-cola from the shell and I've launched it from the repo root and from another directory: in both cases the issue is the same.

lucaotta commented Oct 6, 2017

Unfortunately the repo is not public. I've verified that this happens only on one particular file, for other files the functionality is working well.
The patch file produced is the following:

--- a/C++/Support/FactoryTest/factory_test.cpp
+++ b/C++/Support/FactoryTest/factory_test.cpp
@@ -9,6 +9,7 @@ PowerupStateMachine::PowerupStateMachine(AlarmMng *alarms, bool fctTestDone, boo
     if ( !fctTestDone )
         m_state = FactoryStep1;
     m_alarms = alarms;
+    m_state = Normal;
 }
 
 PowerupStateMachine::States PowerupStateMachine::state() const

I'm on Mac, I launch git-cola from the shell and I've launched it from the repo root and from another directory: in both cases the issue is the same.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Oct 11, 2017

Member

I'm led to believe that git itself might be expecting a slightly different diff header for this hunk. The output above, I presume, is from cola's generated patch file.

If you run git diff on the same file, what does the diff look like from git? Is the header different?

Member

davvid commented Oct 11, 2017

I'm led to believe that git itself might be expecting a slightly different diff header for this hunk. The output above, I presume, is from cola's generated patch file.

If you run git diff on the same file, what does the diff look like from git? Is the header different?

@lucaotta

This comment has been minimized.

Show comment
Hide comment
@lucaotta

lucaotta Oct 19, 2017

I managed to trim down the issue and reproduce it into a public repo:
https://github.com/lucaotta/git-cola-issue-730

lucaotta commented Oct 19, 2017

I managed to trim down the issue and reproduce it into a public repo:
https://github.com/lucaotta/git-cola-issue-730

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Oct 19, 2017

Member

Awesome.. how do I reproduce it, should I just edit the file to add that same line as above? I noticed that the file has CRLF \r\n line endings.. perhaps it's related, we'll see.

when you edit, is your editor adding CRLF, or is it possibly adding LF \n only into a CRLF file? thanks for the reproducer.

Update: adding the line using CRLF just like the rest of the file reproduces the issue. We probably need to better handle files with CRLF line endings.

Member

davvid commented Oct 19, 2017

Awesome.. how do I reproduce it, should I just edit the file to add that same line as above? I noticed that the file has CRLF \r\n line endings.. perhaps it's related, we'll see.

when you edit, is your editor adding CRLF, or is it possibly adding LF \n only into a CRLF file? thanks for the reproducer.

Update: adding the line using CRLF just like the rest of the file reproduces the issue. We probably need to better handle files with CRLF line endings.

davvid added a commit to davvid/git-cola that referenced this issue Oct 19, 2017

doc/relnotes/v3.0: update release notes draft to mention git-cola#730
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this issue Oct 19, 2017

Merge branch 'crlf'
Support files with CRLF line endings when staging interactively.

* crlf:
  diffparse: properly handle files with CRLF line endings
  doc/relnotes/v3.0: update release notes draft to mention git-cola#730
  doc: add Luca to the credits

Closes git-cola#730
Helped-by: Luca Ottaviano <lottaviano@develer.com>
Signed-off-by: David Aguilar <davvid@gmail.com>

@davvid davvid closed this in 95b2e17 Oct 19, 2017

davvid added a commit to davvid/git-cola that referenced this issue Oct 19, 2017

tests: update diffparse tests for new eol semantics
The diff parser now retains the original line endings.
Update tests to expect the new semantics.

Related-to: git-cola#730
Signed-off-by: David Aguilar <davvid@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment