Re: [PATCH 1/5] revision: invert meaning of the USER_GIVEN flag

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 10, 2018, 18:43 UTC
Message-ID: <20180810184341.208183-1-jonathantanmy@google.com>
In-Reply-To: <87799036f82793b691e899bda4fb882a4b231c3a.1533854545.git.matvore@google.com>

Show 8 quoted lines

> Abandon the previous approach of mutating all new objects implicitly in
> add_pending_object by inverting the meaning of the bit (it is now
> NOT_USER_GIVEN) and only setting the flag when we need to.
> 
> This more accurately tracks if a tree was provided directly by the user.
> Without this patch, the root tree of all commits were erroneously
> considered to be USER_GIVEN, which meant they cannot be filtered. This
> distinction is important in the next patch.

After rereading this patch, I think the thought process is:

 - the existing code inaccurately makes root trees of commits USER_GIVEN
 - instead of trying to fix that, it is easier to invert the meaning of this
   flag, and since we only need to track trees and blobs, let's do so in this
   patch

So a better commit message might be:

  revision: mark non-user-given objects instead

  Currently, list-objects.c incorrectly treats all root trees of commits
  as USER_GIVEN. Also, it would be easier to mark objects that are
  non-user-given instead of user-given, since the places in the code
  where we access an object through a reference are more obvious than
  the places where we access an object that was given by the user.

  Resolve these two problems by introducing a flag NOT_USER_GIVEN that
  marks blobs and trees that are non-user-given, replacing USER_GIVEN.
  (Only blobs and trees are marked because this mark is only used when
  filtering objects, and filtering of other types of objects are not
  supported yet.)

The patch itself looks good to me.