Re: [PATCH v7 5/7] revision: mark non-user-given objects instead

[Matthew DeVore <matvore@google.com>]

On Tue, Sep 4, 2018 at 1:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matthew DeVore <matvore@google.com> writes:
>
> > diff --git a/revision.h b/revision.h
> > index 5118aaaa9..2d381e636 100644
> > --- a/revision.h
> > +++ b/revision.h
> > @@ -8,7 +8,11 @@
> >  #include "diff.h"
> >  #include "commit-slab-decl.h"
> >
> > -/* Remember to update object flag allocation in object.h */
> > +/* Remember to update object flag allocation in object.h
> > + * NEEDSWORK: NOT_USER_GIVEN doesn't apply to commits because we only support
> > + * filtering trees and blobs, but it may be useful to support filtering commits
> > + * in the future.
> > + */
>
> Just a minor style nit, but our multi-line comment begins with the
> opening "/*" (and closing "*/", too, but you got that right) on its
> own line, i.e.
>
Fixed.

>         /*
>          * Remember to update ...
>
> > -#define USER_GIVEN   (1u<<25) /* given directly by the user */
> > +#define NOT_USER_GIVEN       (1u<<25) /* tree or blob not given directly by user */
>
> Is "given directly by user" equivalent to "given on the command
> line"?  Do objects given via "--stdin" count the same way?  How abot
> those given via "--branches" or "A^@"?  Does "not given directly by
> user" mean roughly the same thing as "discovered by traversal"?
Note that --branches and A^@ expands to commits, and commits can't be
filtered, so perhaps these questions will be unlikely for the time
being. I did clarify the comment a bit.

I also noticed that this commit fixes a bug - before this patch, "git
rev-list" would potentially filter objects given on the command
line/--stdin, while "git pack-objects" would not. That was because
each command used a different function to populate the rev_info
struct, and rev-list's code path did not contain the hack which turned
on USER_GIVEN.

So I added a test to check the desired behavior. Here is an interdiff
for this commit only:

diff --git a/revision.h b/revision.h
index fe4ff1fec..83e164039 100644
--- a/revision.h
+++ b/revision.h
@@ -9,11 +9,7 @@
 #include "diff.h"
 #include "commit-slab-decl.h"

-/* Remember to update object flag allocation in object.h
- * NEEDSWORK: NOT_USER_GIVEN doesn't apply to commits because we only support
- * filtering trees and blobs, but it may be useful to support filtering commits
- * in the future.
- */
+/* Remember to update object flag allocation in object.h */
 #define SEEN (1u<<0)
 #define UNINTERESTING   (1u<<1)
 #define TREESAME (1u<<2)
@@ -25,7 +21,14 @@
 #define SYMMETRIC_LEFT (1u<<8)
 #define PATCHSAME (1u<<9)
 #define BOTTOM (1u<<10)
-#define NOT_USER_GIVEN (1u<<25) /* tree or blob not given directly by user */
+/*
+ * Indicates object was reached by traversal. i.e. not given by user on
+ * command-line or stdin.
+ * NEEDSWORK: NOT_USER_GIVEN doesn't apply to commits because we only support
+ * filtering trees and blobs, but it may be useful to support filtering commits
+ * in the future.
+ */
+#define NOT_USER_GIVEN (1u<<25)
 #define TRACK_LINEAR (1u<<26)
 #define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)

diff --git a/t/t6112-rev-list-filters-objects.sh
b/t/t6112-rev-list-filters-objects.sh
index 2e6a6a32e..a989a7082 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -30,6 +30,16 @@ test_expect_success 'verify blob:none omits all 5 blobs' '
  test_cmp observed expected
 '

+test_expect_success 'specify blob explicitly prevents filtering' '
+ file_3=$(git -C r1 ls-files -s file.3 \
+ | awk -f print_2.awk) &&
+ file_4=$(git -C r1 ls-files -s file.4 \
+ | awk -f print_2.awk) &&
+ git -C r1 rev-list HEAD --objects --filter=blob:none HEAD $file_3 >observed &&
+ grep -q "$file_3" observed &&
+ test_must_fail grep -q "$file_4" observed
+'
+
 test_expect_success 'verify emitted+omitted == all' '
  git -C r1 rev-list HEAD --objects \
  | awk -f print_1.awk \

>
> Not a suggestion to change anything in this patch, but if you can
> come up with a better phrase that helps new readers' understanding
> so that they do not have to ask a question like this, that would be
> great.
>
> Thanks.
>