Dealing with rejection

by David Humphrey

Chris Tyler's SBR600 student, Boris Chao, is the kind of fearless student I love.  A week ago we approached him to see if he'd be interested to work on doing a backport of the new plugin code in GCC 4.5 to GCC 4.4.  Without really knowing what he was in for, he said yes, and started working.  As he's gone along he's hit some bumps, and in every case used those as catalysts for learning new things.  What he's doing isn't really obvious or easy, but it's also not hard--it's just a bunch of tools and processes that are new to most students, and many developers.

I was all set to write a post today about backporting, svn diff, patch, etc. and then I see that he's beat me to it!  However, I thought I'd add a bit more to what he's started and talk about how one deals with patches that don't go in cleanly.

First, a brief word about patches.  Patches are text files that describe differences between two versions of a file or set of files.  They are typically created with some form of the diff command, either using the stand-alone diff command or via your revision control system (e.g., svn diff, hg diff, ...).  Patches are designed to make it possible for me to do work locally, and share my changes with others (i.e, on a mailing list or in a bug) without having to send them everything I did.  A patch is then applied using the patch command, hence the name.  So when people talk about 'diffs' and 'patches,' they mean the same thing.

I happen to be looking at a patch right now for another bug I'm following, so let's use that as an example.  In this patch (which was created using the hg diff command), you can see a few things.  First, you can see that there are two hunks.  A hunk is a change to a part of a file, and a patch can contain many of them.  Here's the first hunk:

diff -r d76583175408 accessible/src/atk/nsAccessibleWrap.cpp  
--- a/accessible/src/atk/nsAccessibleWrap.cpp	Wed Nov 25 02:40:46 2009 -0500  
+++ b/accessible/src/atk/nsAccessibleWrap.cpp	Thu Nov 26 16:58:34 2009 -0500  
@@ -342,16 +342,24 @@ void nsAccessibleWrap::SetMaiHyperlink(M  
         if (maiHyperlink) {  
             delete maiHyperlink;  
         }  
         g_object_set_qdata(G_OBJECT(mAtkObject), quark_mai_hyperlink,  
                            aMaiHyperlink);  
     }  
 }  
  
+const char *  
+nsAccessibleWrap::ReturnString(nsAString &aString)  
+{  
+  static nsCString returnedString;  
+  returnedString = NS_ConvertUTF16toUTF8(aString);  
+  return returnedString.get();  
+}  
+  
 NS_IMETHODIMP nsAccessibleWrap::GetNativeInterface(void **aOutAccessible)  
 {  
     *aOutAccessible = nsnull;  
  
     if (!mAtkObject) {  
         if (!mWeakShell || !nsAccUtils::IsEmbeddedObject(this)) {  
             // We don't create ATK objects for node which has been shutdown, or  
             // nsIAccessible plain text leaves

You can see that this hunk relates to the file, accessible/src/atk/nsAccessibleWrap.cpp.  Next, you can see that, in particular, it deals with the following section (or range) of code:

@@ -342,16 +342,24 @@ void nsAccessibleWrap::SetMaiHyperlink(M

The @@ -342,16 +342,24 @@ is the chunk's range information.  This says that in the original file, this chunk begins at line 342, and goes for 16 lines.  In the patched version of the file (i.e., after applying these changes), it is line 342 and goes for 24 lines.  These two ranges are needed so that multiple hunks to the same file can take into account proper offsets from applying previous changes.

The hunk above takes code from line 342 to 358 (342 + 16) and shows it for context.  Patches always have some amount of context (Mozilla uses 8 lines as a default, some projects use 3).  The more context you have, the easier it is to figure out where this change should go.  This hunk then specifies that  8 new lines need to get inserted, and indicates them with a + sign.  There are three characters that can begin a line in a hunk:

  1. [space] this is a line of context (nothing will be changed)
    • this is a line that needs to be inserted
    • this is a line that needs to be removed.
      Now, take a look at the second hunk in this patch:
@@ -106,21 +106,19 @@ public:  
     static AtkObject * GetAtkObject(nsIAccessible * acc);  
  
     PRBool IsValidObject();  
  
     // get/set the MaiHyperlink object for this nsAccessibleWrap  
     MaiHyperlink* GetMaiHyperlink(PRBool aCreate = PR_TRUE);  
     void SetMaiHyperlink(MaiHyperlink* aMaiHyperlink);  
  
-    static const char * ReturnString(nsAString &aString) {  
-      static nsCString returnedString;  
-      returnedString = NS_ConvertUTF16toUTF8(aString);  
-      return returnedString.get();  
-    }  
+    // member function cannot be inline due to a known bug in GCC 4.5  
+    // see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42171 for details  
+    static const char * ReturnString(nsAString &aString);  
  
 protected:  
     virtual nsresult FirePlatformEvent(nsIAccessibleEvent *aEvent);  
  
     nsresult FireAtkStateChangeEvent(nsIAccessibleEvent *aEvent,  
                                      AtkObject *aObject);  
     nsresult FireAtkTextChangedEvent(nsIAccessibleEvent *aEvent,  
                                      AtkObject *aObject);

Here you can see 5 lines being removed, and 3 new lines being added in their place.  This is what an edit looks like in a patch.

This patch is pretty easy to understand once you know the format of a diff file: take this file, find this line, add these lines, remove these lines.  But, it's not so simple.  When you apply this patch (you'd do it like this: patch -p1 < patchfile), here is what patch will try and do (from the patch MAN page):

For each hunk, the patch utility begins to search for the place to apply the patch at the line number at the beginning of the hunk, plus or minus any offset used in applying the previous hunk. If lines matching the hunk context are not found, patch scans both forwards and backwards at least 1000 bytes for a set of lines that match the hunk context.

If no such place is found and it is a context difference, then another scan will take place, ignoring the first and last line of context. If that fails, the first two and last two lines of context will be ignored and another scan will be made. Implementations may search more extensively for installation locations.

If no location can be found, the patch utility will append the hunk to the reject file.
Now we're to the part I wanted to write about.  In Boris' post, he shows output from applying a patch and having hunks fail.  This happens when patch can't locate the place to make your change, or can locate it but it differs so much so that it can't decide what to do.  Patches that don't apply cleanly are said to have "bit rotted," meaning that the version you made your patch against is now old, and new changes to the file keep it from applying cleanly.  When this happens, you end-up with a file named original-file.rej.

The reject file is a list of all hunks that failed to apply for this file.  As such, it is simply a patch.  The thing to do with a failed patch is to open the file you're trying to patch along with the .rej file, and see what's going on.  The hunk will tell you the context for the change, plus which lines need to change.  What you need to do is look at those changes, find the right spot in this file, and make them manually.  You might also decide to ignore them.  Whatever you do, once you've addressed all the failed hunks in your file, you can delete the .rej file.

This process seems intimidating at first, but is really not that complicated.  There are also tools to help with this (e.g., some editors understand diff/patch, and graphical 3-way merge tools will save you much hair pulling).  More than anything, it's just a bit of surgery to carefully remove and add lines of code.  Don't be afraid of it, just take your time.