Merge lp:~ajeans/bzr/fix-40103-status-on-ignored into lp:bzr

[Arnaud Jeansen]

Inform users that a file specified to commit is an ignored unversioned file.

This branch adds a warning when the user issues a status command on a given file, but the file is unversioned *and* should be ignored. A unit test is added to show the new behaviour

LP: #40103 has more details

[Martin Pool (mbp)]

Thanks for tackling this and thanks for giving a good cover letter including the bug link - it makes it much easier to review.

I think this is a bit overspecific to the case of giving just one ignored file; if I said

 bzr st foo~ bar~

and they were both ignored I would still find it strange if this just said nothing.

Now in the output format perhaps it is better not to give a warning but rather to show them under the heading 'ignored':

  % bzr st foo~ bar~
  ignored:
    foo~
    bar~

that might be more adaptable to other presentations of the status output too.

hth

review: Needs Fixing

[Arnaud Jeansen (ajeans)]

Thanks for the quick reply Martin,

I was looking for an easy fix to a very specific case. If we want to be complete, we need to answer the following questions (where 'we' translates to 'you' :))

1) "ignored:\n foo~\n bar~\n" looks similar to the "normal" output, so to be consistent we should also handle short output with something like "I foo~\nI bar~". Does it look correct to you ?

2) If we output a list of ignored files, in which conditions do we make it appear ?
   a) Display ignored files whatever the case (even for a general "bzr status")
   b) Display ignored files when specific files have been fed (only for "bzr status foo~ bar~")
   c) Display ignored files when specifc files have been fed, and no other status is present (for "bzr status foo*" and only if there is not a versioned 'foo' file)

I would be in favor of 1)handling short and 2)c), which is the less intrusive change.

Thoughts ?
Arnaud

[Martin Pool (mbp)]

On 1 April 2010 03:17, Arnaud Jeansen <email address hidden> wrote:
> Thanks for the quick reply Martin,
>
> I was looking for an easy fix to a very specific case. If we want to be complete, we need to answer the following questions (where 'we' translates to 'you' :))
>
> 1) "ignored:\n foo~\n bar~\n" looks similar to the "normal" output, so to be consistent we should also handle short output with something like "I  foo~\nI  bar~". Does it look correct to you ?
>
> 2) If we output a list of ignored files, in which conditions do we make it appear ?
>   a) Display ignored files whatever the case (even for a general "bzr status")
>   b) Display ignored files when specific files have been fed (only for "bzr status foo~ bar~")
>   c) Display ignored files when specifc files have been fed, and no other status is present (for "bzr status foo*" and only if there is not a versioned 'foo' file)
>
> I would be in favor of 1)handling short and 2)c), which is the less intrusive change.

I would have said 1 and 2b, if I'm understanding you correctly. All
the files you name on the command line should be included in the
output, even if some or all of them are ignored. That seems probably
simplest for the user.

--
Martin <http://launchpad.net/~mbp/>

[Arnaud Jeansen (ajeans)]

Martin,

Understood, I will give this a try. However, I will wait for the status callback to land (https://code.launchpad.net/~ajeans/bzr/status-callback/+merge/20828). It will make the change easier for me, and it touches the same code (now that the change impacts normal and short output).

Thanks.

[Ian Clatworthy (ian-clatworthy)]

Hi Arnaud,

The status-callback branch has now landed.

Could you mark this patch as Work In Progress if it's back in your court (rather than needing another review) please?

[Arnaud Jeansen (ajeans)]

Good evening,

The merge interface does not seem to like a push --overwrite on a rebased branch, sorry about this mess. Obviously only changes authored by me are relevant. At least, the diff below is clean.

Martin, I implemented what you suggested in your latest comment. I'm not entirely happy with the code (pushing this change outside of the new callback and the ChangeReporter), but I think this is the best 'short term' fix.

I will try to clean this up along with the rest, but that will be RFC material.

Thank you,
Arnaud

[Robert Collins (lifeless)]

I'm not sure the XXX is appropriate; deltas can't really talk about ignored/not ignored, and I'm not sure they should. Could you take it out? I find speculative XXX's age very fast and usually just are noise.

review: Needs Fixing

[Arnaud Jeansen (ajeans)]

Good evening Robert,

I don't really mind removing the XXX, so here is the change.

However I am convinced that this whole part should be improved, and I will submit a proposal as time permits.

Thank you for the review.

[Robert Collins (lifeless)]

Queued on 2010-04-15 23:30:35.997048+00:00 now submitted to PQM.

=== modified file 'bzrlib/status.py'
--- bzrlib/status.py	2010-03-20 00:07:45 +0000
+++ bzrlib/status.py	2010-04-15 20:25:58 +0000
@@ -166,6 +166,20 @@
                            short=short, want_unchanged=show_unchanged, 
                            want_unversioned=want_unversioned, show_ids=show_ids)
 
+            # show the ignored files among specific files (i.e. show the files
+            # identified from input that we choose to ignore). 
+            if specific_files is not None:
+                # Ignored files is sorted because specific_files is already sorted
+                ignored_files = [specific for specific in
+                    specific_files if new.is_ignored(specific)]
+                if len(ignored_files) > 0 and not short:
+                    to_file.write("ignored:\n")
+                    prefix = ' '
+                else:
+                    prefix = 'I  '
+                for ignored_file in ignored_files:
+                    to_file.write("%s %s\n" % (prefix, ignored_file))
+
             # show the new conflicts only for now. XXX: get them from the
             # delta.
             conflicts = new.conflicts()

=== modified file 'bzrlib/tests/blackbox/test_status.py'
--- bzrlib/tests/blackbox/test_status.py	2010-03-25 09:39:03 +0000
+++ bzrlib/tests/blackbox/test_status.py	2010-04-15 20:25:58 +0000
@@ -472,6 +472,39 @@
         self.assertEqual("working tree is out of date, run 'bzr update'\n",
                          err)
 
+    def test_status_on_ignored(self):
+        """Tests branch status on an unversioned file which is considered ignored.
+
+        See https://bugs.launchpad.net/bzr/+bug/40103
+        """
+        tree = self.make_branch_and_tree('.')
+
+        self.build_tree(['test1.c', 'test1.c~', 'test2.c~'])
+        result = self.run_bzr('status')[0]
+        self.assertContainsRe(result, "unknown:\n  test1.c\n")
+        short_result = self.run_bzr('status --short')[0]
+        self.assertContainsRe(short_result, "\?   test1.c\n")
+
+        result = self.run_bzr('status test1.c')[0]
+        self.assertContainsRe(result, "unknown:\n  test1.c\n")
+        short_result = self.run_bzr('status --short test1.c')[0]
+        self.assertContainsRe(short_result, "\?   test1.c\n")
+
+        result = self.run_bzr('status test1.c~')[0]
+        self.assertContainsRe(result, "ignored:\n  test1.c~\n")
+        short_result = self.run_bzr('status --short test1.c~')[0]
+        self.assertContainsRe(short_result, "I   test1.c~\n")
+
+        result = self.run_bzr('status test1.c~ test2.c~')[0]
+        self.assertContainsRe(result, "ignored:\n  test1.c~\n  test2.c~\n")
+        short_result = self.run_bzr('status --short test1.c~ test2.c~')[0]
+        self.assertContainsRe(short_result, "I   test1.c~\nI   test2.c~\n")
+
+        result = self.run_bzr('status test1.c test1.c~ test2.c~')[0]
+        self.assertContainsRe(result, "unknown:\n  test1.c\nignored:\n  test1.c~\n  test2.c~\n")
+        short_result = self.run_bzr('status --short test1.c test1.c~ test2.c~')[0]
+        self.assertContainsRe(short_result, "\?   test1.c\nI   test1.c~\nI   test2.c~\n")
+
     def test_status_write_lock(self):
         """Test that status works without fetching history and
         having a write lock.