Re: [PATCH DIFF-CLEANUP 1/2] Make the diff_options bitfields be an unsigned with explicit masks.

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 2688 bytes --]

On Thu, Nov 08, 2007 at 06:39:02AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > reverse_diff was a bit-value in disguise, it's merged in the flags now.
> >
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> 
> Just my first impression, as I am in the middle of unrelated
> bisect.  I haven't read beyond diff-lib.c changes.
> 
> > diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
> > index 0b591c8..e71841a 100644
> > --- a/builtin-diff-tree.c
> > +++ b/builtin-diff-tree.c
> > @@ -118,12 +118,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
> >  	}
> >  
> >  	if (!read_stdin)
> > -		return opt->diffopt.exit_with_status ?
> > -		    opt->diffopt.has_changes: 0;
> > +		return DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)
> > +			&& DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES);
> 
> Had to think a bit about this, although it is correct.
> 
> >  	if (opt->diffopt.detect_rename)
> >  		opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
> > -				       DIFF_SETUP_USE_CACHE);
> > +							   DIFF_SETUP_USE_CACHE);
> 
> I wonder what this is about.

  err I code with tabs of size 4 and I believe my editor was
over-zealous when I asked to reindent some part that I changed :P

> > diff --git a/combine-diff.c b/combine-diff.c
> > index fe5a2a1..3cab04b 100644
> > --- a/combine-diff.c
> > +++ b/combine-diff.c
> > @@ -664,7 +664,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
> >  	int mode_differs = 0;
> >  	int i, show_hunks;
> >  	int working_tree_file = is_null_sha1(elem->sha1);
> > -	int abbrev = opt->full_index ? 40 : DEFAULT_ABBREV;
> > +        int abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;
> 
> Indent?

  will fix.

> > diff --git a/diff-lib.c b/diff-lib.c
> > index da55713..69b5dc9 100644
> > --- a/diff-lib.c
> > +++ b/diff-lib.c
> > @@ -188,8 +188,7 @@ static int handle_diff_files_args(struct rev_info *revs,
> >  		else if (!strcmp(argv[1], "-n") ||
> >  				!strcmp(argv[1], "--no-index")) {
> >  			revs->max_count = -2;
> > -			revs->diffopt.exit_with_status = 1;
> > -			revs->diffopt.no_index = 1;
> > +			revs->diffopt.flags |= DIFF_OPT_EXIT_WITH_STATUS | DIFF_OPT_NO_INDEX;
> >  		}
> 
> Now this looks harder to read that everybody else uses
> DIFF_OPT_SET() for this, without DIFF_OPT_ prefix for the
> bitmask names.

  that could be splitted in two DIFF_OPT_SET indeed. will do.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]