On Tue, 5 Feb 2008, Johannes Schindelin wrote:
> Hi,
>
> On Tue, 5 Feb 2008, Daniel Barkalow wrote:
>
> > On Tue, 5 Feb 2008, Johannes Schindelin wrote:
> >
> > So, as an introductory comment, I was working quite strictly from
> > git-checkout.sh, using the implementations of the operations as given
> > there, and not looking at what other code has the same goal and may be
> > doing it differently, so I've almost certainly missed a number of
> > chances for sharing code (beyond calling functions directly instead of
> > putting together command lines and having them parsed). In particular, I
> > hadn't looked at all at builtin-reset, which obviously does a bunch of
> > the same things.
>
> Well, it seems like I won't be able to review any more til this weekend.
> Sorry.
It's okay, I've got other things to work on. I'll probably send out a new
version of the series now, in any case, so that you can see if I handled
your comments correctly thus far.
> > > > + hashcpy(ce->sha1, sha1);
> > > > + memcpy(ce->name, base, baselen);
> > > > + memcpy(ce->name + baselen, pathname, len - baselen);
> > > > + ce->ce_flags = create_ce_flags(len, 0);
> > > > + ce->ce_mode = create_ce_mode(mode);
> > > > + add_cache_entry(ce, ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int read_tree_some(struct tree *tree, const char **pathspec)
> > >
> > > I have a hunch that you could share code with builtin-reset.c, namely
> > > call function read_from_tree() here.
> >
> > This version matches the "ls-tree | update-index" implementation in
> > git-checkout.sh; I'm not terribly fond of the diff-index-based version,
> > since it messed up the index, rereads it from disk, and then applies
> > changes. On the other hand, it would probably be good to update the
> > cache entries with a function that would keep the stat info if nothing
> > else changed.
>
> Exactly. In fact, that is the only reason I suggested that: to prevent
> the index from needing an update.
>
> This is quite important if you have a large working tree, and switch
> branches from A to B. For example, "make" will punish you where it hurts.
This isn't used for switching branches; this is used for checking out
paths. If you do "git checkout <not-head> -- <every single path>", make
will punish you, but why would you do that? I'd guess that people are
unlikely to have a significant number of non-changes in this piece of
code, just because they wouldn't list things that they didn't think had
changes.
> > > > +static void show_local_changes(struct object *head)
> > > > +{
> > > > + struct rev_info rev;
> > > > + // I think we want full paths, even if we're in a subdirectory.
> > >
> > > Please avoid C++-style comments.
> >
> > Yup. Can we make git build by default with --std=c89, so I get errors for
> > this sort of thing? *sigh*
>
> Heh. You can do that in config.mak. For the Makefile, I am afraid this
> is no option, as we try to support other compilers than gcc, too.
I'd say only for the case where CC is initially unset, at which point the
compiler is going to be "gcc" because we set it to that. And people who
symlink their non-gcc-like system compiler to gcc have other problems,
most likely.
> > > > +static int reset_to_new(struct tree *tree, int quiet)
> > >
> > > Again, I think that this would benefit from code sharing with
> > > builtin-reset. It can be a bit tricky to avoid writing files when they
> > > are already up-to-date...
> >
> > Reset actually execs "read-tree", which does a bunch of parsing to set up
> > the arguments like this function does (except that it doesn't keep the
> > stat info, I think), and then uses the same actual code that this function
> > calls directly (which has done the only-write-changes thing correctly
> > since June 2005 or before).
>
> Okay, I'll just wait and see (as I do not have the time to dive into the
> code).
Sure. I'll probably actually change builtin-reset to do the non-fork
thing (in a separate series; it's really unrelated). I doubt there's any
point to sharing the code for setting all of the options, though, which is
best to have inlined by the compiler and be easy to find.
-Daniel
*This .sig left intentionally blank*
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
