Proposed git mv behavioral change

Proposed git mv behavioral change

[lmage11]

Hey, all...

Based on a question I asked on freenode's #git channel a few days ago, it seems that most 
people only use git 
mv when the file they're moving is clean. Those that don't have become accustomed to the 
behavior that git-mv 
uses when the file is dirty - to pull all unstaged changes into the index. As far as I can tell, 
this behavior is 
largely historical. When git-mv was written in bash (or perl, or whatever it was initially written 
in), it was most 
convenient to implement it as "mv oldname newname; git add newname;", or something 
similar, which would 
result in pulling in all wd changes. However, it seems to me that this behavior violates git's 
consistency. Usually 
when I do something with git, I expect it to do only what it says it will do, and nothing more. 
The 
documentation for git-mv says "The index is updated after successful completion", but I 
would tend to assume 
that this was referring to the name change and not the change in the actual contents of the 
file.

In my situation, I have some changes to a file that I'm not yet ready to commit. In the 
meantime, I've started 
working on another unrelated change that involves renaming one of the files which contain 
unstaged changes. 
I'd like to be able to perform this move without having my unstaged changes implicitly 
staged without my 
knowledge. When I want information put into the index, I either use git update-index if I 
want to explicitly 
stage an entire file or I use git add -i's patch command to explicitly stage individual chunks. I 
like having very 
fine-grained control over what goes into the index and what doesn't.

Therefore, I propose that git mv's behavior be changed. I think it would make far more sense 
for a move to only 
change the actual name of the file and to not pull in unstaged changes. In other words, I'd 
like the index entry 
for the original file name to be removed and an index entry to be added with a different 
name, but using the 
exact same blob hash as the original file. I don't know exactly how git manages the index 
internally, but a 
shortcut for this would be to simply rename the index entry in place.

I'm willing to look into what changes would need to be made to the code for this change to 
happen; I'm not 
asking for someone to do all the work for me. :)

So... Yeah. I'd like to know what people think about this before I put a significant amount of 
effort into it. After 
all, we know how lazy programmers are... :)

Thanks,
   Ari

Re: Proposed git mv behavioral change

[Michael Witten]

On 18 Oct 2007, at 11:47:32 AM, lmage11@twcny.rr.com wrote:

> I don't know exactly how git manages the index
> internally, but a
> shortcut for this would be to simply rename the index entry in place.

Seems like the shortcut would lose the history and confuse git.

Anyway,

Let's say you have a file, A.txt, with contents:

	A

You edit this file so that it has contents:
	
	A_dirty

You're saying that, currently, 'git-mv A.txt path/B.txt'
does this:
	
	mv A.txt path/B.txt
	git add path/B.txt
	git rm  A.txt

So that A.txt is indeed moved to path/B.txt, but now
A_dirty has been added to the index.

What you want to happen is the following:
	
	git show HEAD:A.txt > path/B.txt
	git add path/B.txt
	mv A.txt B.txt
	git rm A.txt

Is this correct?

Michael Witten

Re: Proposed git mv behavioral change

[Shawn O. Pearce]

Michael Witten <mfwitten@MIT.EDU> wrote:
> On 18 Oct 2007, at 11:47:32 AM, lmage11@twcny.rr.com wrote:
> 
> >I don't know exactly how git manages the index
> >internally, but a
> >shortcut for this would be to simply rename the index entry in place.
> 
> Seems like the shortcut would lose the history and confuse git.

No.  It wouldn't.  The index has no knowledge of history of anything.
For good reason.  Its strictly a mapping of file name to some basic
stat information (used to determine if the file has been modified
by the user or not) and the SHA-1 of the staged blob.  That's it.
No history.  The shortcut the original poster was asking about
wouldn't confuse Git one bit.
 
-- 
Shawn.

Re: Proposed git mv behavioral change

[Michael Witten]

On 18 Oct 2007, at 9:57:15 PM, Shawn O. Pearce wrote:

> Michael Witten <mfwitten@MIT.EDU> wrote:
>> On 18 Oct 2007, at 11:47:32 AM, lmage11@twcny.rr.com wrote:
>>
>>> I don't know exactly how git manages the index
>>> internally, but a
>>> shortcut for this would be to simply rename the index entry in  
>>> place.
>>
>> Seems like the shortcut would lose the history and confuse git.
>
> No.  It wouldn't.  The index has no knowledge of history of anything.

I mean to say, if only the index is changed,
then git won't be informed about the necessary
git-{add/rm}'s, as in the following (is this
not so?):

> You're saying that, currently, 'git-mv A.txt path/B.txt'
> does this:
> 	
> 	mv A.txt path/B.txt
> 	git add path/B.txt
> 	git rm  A.txt
>
> So that A.txt is indeed moved to path/B.txt, but now
> A_dirty has been added to the index.
>
> What you want to happen is the following:
> 	
> 	git show HEAD:A.txt > path/B.txt
> 	git add path/B.txt
> 	mv A.txt B.txt
> 	git rm A.txt

Re: Proposed git mv behavioral change

[Shawn O. Pearce]

Michael Witten <mfwitten@MIT.EDU> wrote:
> On 18 Oct 2007, at 9:57:15 PM, Shawn O. Pearce wrote:
> >Michael Witten <mfwitten@MIT.EDU> wrote:
> >>
> >>Seems like the shortcut would lose the history and confuse git.
> >
> >No.  It wouldn't.  The index has no knowledge of history of anything.
> 
> I mean to say, if only the index is changed,
> then git won't be informed about the necessary
> git-{add/rm}'s, as in the following (is this
> not so?):

git-add amounts to either inserting a new path->stat/sha1 entry
in the index, or updating an existing entry with new stat/sha1
information.

git-rm amounts to removing a path->stat/sha1 entry from the index.
It's just gone once the git-rm is completed.  As if it was never
there to begin with.

A git-commit (really git-write-tree but same difference to the
end-user) stores whatever is in the index as the gospel truth for
how that commit's files should appear.  No knowledge about add
or rm is necessary at this stage, we're just taking a copy of the
index and recording that for posterity.

So updating the index is all that is necessary to "remember" these
add and rm operations.  OK, well, you also need to actually make
a commit (and not orphan that commit) to have it really stay in
your project.  But its all really as simple as it seems.

-- 
Shawn.

Re: Proposed git mv behavioral change

[Jeff King]

On Thu, Oct 18, 2007 at 10:07:05PM -0400, Michael Witten wrote:

> I mean to say, if only the index is changed,
> then git won't be informed about the necessary
> git-{add/rm}'s, as in the following (is this
> not so?):

Eh? The changes will make it into history when you commit. Or perhaps I
don't understand your question. Can you rephrase it?

-Peff

Re: Proposed git mv behavioral change

[Michael Witten]

On 18 Oct 2007, at 10:15:29 PM, Shawn O. Pearce wrote:
> git-add amounts to either inserting a new path->stat/sha1 entry
> in the index, or updating an existing entry with new stat/sha1
> information.
>
> git-rm amounts to removing a path->stat/sha1 entry from the index.
> It's just gone once the git-rm is completed.  As if it was never
> there to begin with.
>
> A git-commit (really git-write-tree but same difference to the
> end-user) stores whatever is in the index as the gospel truth for
> how that commit's files should appear.

Ah. Basically my 'pseudo-code' is correct, but redundant.


On 18 Oct 2007, at 10:16:18 PM, Jeff King wrote:
> Eh? The changes will make it into history when you commit. Or  
> perhaps I
> don't understand your question. Can you rephrase it?

I misunderstood the (lack) of details.

On 18 Oct 2007, at 10:15:29 PM, Shawn O. Pearce wrote:
> But its all really as simple as it seems.

It's about time I read more thoroughly through the manual!

Thanks.

Re: Proposed git mv behavioral change

[Wincent Colaiuta]

El 19/10/2007, a las 4:29, Michael Witten escribió:

> Ah. Basically my 'pseudo-code' is correct, but redundant.

If I understood the original poster's proposal then I don't think  
your code does what he asked for:

> What you want to happen is the following:
> 	
> 	git show HEAD:A.txt > path/B.txt
> 	git add path/B.txt
> 	mv A.txt B.txt
> 	git rm A.txt
>
> Is this correct?

Here you're copying the content of A.txt as it was in the last (HEAD)  
commit, but from what the poster said he wants the content of A.txt  
as it is staged in the index (that is, there may be staged but  
uncomitted changes).

> Better:
>
>>  	mv A.txt path/B.txt
>> 	Point the index entry for A.txt to path/B.txt

Yes, that is basically what he was asking for, as I read it.

El 19/10/2007, a las 5:47, Jeff King escribió:

> Hrm. So you _do_ want to do an index-only move of A to B, in which  
> case
> the suggestion of a "git-mv --cached" seems sensible. Though I'm  
> curious
> why you want that.

I agree that git-stash can be used in this workflow but I can also  
imagine cases where the proposed "git-mv --cached" might be a bit  
nicer. I'm thinking of occasions where you just want to do something  
like:

   git mv --cached foo bar
   git add --interactive bar

I'm not sure the proposed "--cached" switch should ever be the  
default -- would need to ponder that one -- but I do think the switch  
would be a nice addition.

Cheers,
Wincent

Re: Proposed git mv behavioral change

[Michael Witten]

On 19 Oct 2007, at 7:33:39 AM, Wincent Colaiuta wrote:

>> What you want to happen is the following:
>> 	
>> 	git show HEAD:A.txt > path/B.txt
>> 	git add path/B.txt
>> 	mv A.txt B.txt
>> 	git rm A.txt
>>
>> Is this correct?
>
> Here you're copying the content of A.txt as it was in the last  
> (HEAD) commit, but from what the poster said he wants the content  
> of A.txt as it is staged in the index (that is, there may be staged  
> but uncomitted changes).
>
>> Better:
>>
>>>  	mv A.txt path/B.txt
>>> 	Point the index entry for A.txt to path/B.txt
>
> Yes, that is basically what he was asking for, as I read it.

You're right.

There is the subtlety in the first case that he's already staged  
something.

Re: Proposed git mv behavioral change

[Shawn O. Pearce]

Line wrapping email at under 80 columns would be nice.  It makes it
easier to read the message, and more importantly, easier to quote
a few times during discussion.

lmage11@twcny.rr.com wrote:
> Therefore, I propose that git mv's behavior be changed. I think it
> would make far more sense for a move to only change the actual name
> of the file and to not pull in unstaged changes. In other words,
> I'd like the index entry for the original file name to be removed
> and an index entry to be added with a different name, but using the
> exact same blob hash as the original file. I don't know exactly how
> git manages the index internally, but a shortcut for this would be
> to simply rename the index entry in place.

I'm somewhat hesistant to change existing behavior, as users may
be used to it or relying upon it within their scripts.  But you
make an excellent argument about why the current git-mv behavior
is perhaps less than ideal.

Elsewhere in git we use the --cached command line option to mean
"only make the change in the index".  For example the git-apply
--cached option.  You could start a patch that uses --cached to
trigger the new behavior you propose and see if people are interested
in changing the default once the feature is working and available
for experimentation.
 
> I'm willing to look into what changes would need to be made to the
> code for this change to happen; I'm not asking for someone to do
> all the work for me. :)
> 
> So... Yeah. I'd like to know what people think about this before
> I put a significant amount of effort into it. After all, we know
> how lazy programmers are... :)

See builtin-mv.c around l.264-283.  This is where we are removing
the old names from the index (in memory) and inserting the new
names.  Instead of calling add_file_to_cache() you would want
to use something like add_cacheinfo() in builtin-update-index.c,
specifying the old sha1, ce_flags and ce_mode.

I'm sure Junio could probably give you a better starting point
than I can, as he's more familiar with this sort of code, but that
should still get you looking in the right direction and maybe get
a working implementation together that you can share for discussion.

-- 
Shawn.

Re: Proposed git mv behavioral change

[Michael Witten]

On 18 Oct 2007, at 9:54:19 PM, Shawn O. Pearce wrote:

> Elsewhere in git we use the --cached command line option to mean
> "only make the change in the index".


It seems like --cached should be phased out in favor of --index/ed

Re: Proposed git mv behavioral change

[Shawn O. Pearce]

Michael Witten <mfwitten@MIT.EDU> wrote:
> 
> On 18 Oct 2007, at 9:54:19 PM, Shawn O. Pearce wrote:
> 
> >Elsewhere in git we use the --cached command line option to mean
> >"only make the change in the index".
> 
> It seems like --cached should be phased out in favor of --index/ed

No, --index is something else.

But I was originally *way* wrong to propose --cached for this usage
in git-mv.  --cached means "apply *ONLY* to the index" and "do *NOT*
touch the working tree".  Here we want to touch the working tree
in the sense of moving the file.  So --cached is not the correct
option name.

--index is used in Git for places were we update *both* the index
and the working directory (git-apply --index).  So actually I should
have suggested "git-mv --index".  Whoops.

-- 
Shawn.

Re: Proposed git mv behavioral change

[Jeff King]

On Thu, Oct 18, 2007 at 11:19:59PM -0400, Shawn O. Pearce wrote:

> touch the working tree".  Here we want to touch the working tree
> in the sense of moving the file.  So --cached is not the correct
> option name.

Doesn't "mv" do that?

-Peff

Re: Proposed git mv behavioral change

[Michael Witten]

On 18 Oct 2007, at 11:24:07 PM, Jeff King wrote:

> On Thu, Oct 18, 2007 at 11:19:59PM -0400, Shawn O. Pearce wrote:
>
>> touch the working tree".  Here we want to touch the working tree
>> in the sense of moving the file.  So --cached is not the correct
>> option name.
>
> Doesn't "mv" do that?

We only want to move it, not also add its dirty contents.

Re: Proposed git mv behavioral change

[Jeff King]

On Thu, Oct 18, 2007 at 11:26:32PM -0400, Michael Witten wrote:

>>> touch the working tree".  Here we want to touch the working tree
>>> in the sense of moving the file.  So --cached is not the correct
>>> option name.
>>
>> Doesn't "mv" do that?
>
> We only want to move it, not also add its dirty contents.

Right, I mean regular "mv", not "git-mv". The only thing that doesn't
accomplish is moving the "from" entry in the index to the "to" entry
(but I'm not sure that's such a good idea).  Perhaps I've lost sight of
your original goal. Can you state more succintly what you are trying to
accomplish?

-Peff

Re: Proposed git mv behavioral change

[Michael Witten]

On 18 Oct 2007, at 11:35:00 PM, Jeff King wrote:

> Right, I mean regular "mv", not "git-mv". The only thing that doesn't
> accomplish is moving the "from" entry in the index to the "to" entry
> (but I'm not sure that's such a good idea).  Perhaps I've lost  
> sight of
> your original goal. Can you state more succintly what you are  
> trying to
> accomplish?

I think you're right.

Anyway, succinctly:

> What you want to happen is the following:
> 	
> 	git show HEAD:A.txt > path/B.txt
> 	git add path/B.txt
> 	mv A.txt B.txt
> 	git rm A.txt

Better:

>  	mv A.txt path/B.txt
> 	Point the index entry for A.txt to path/B.txt

I hope that's right.

Re: Proposed git mv behavioral change

[Jeff King]

On Thu, Oct 18, 2007 at 11:40:47PM -0400, Michael Witten wrote:

> Anyway, succinctly:
>
>> What you want to happen is the following:
>> 	
>> 	git show HEAD:A.txt > path/B.txt
>> 	git add path/B.txt
>> 	mv A.txt B.txt
>> 	git rm A.txt
>
> Better:
>
>>  	mv A.txt path/B.txt
>> 	Point the index entry for A.txt to path/B.txt
>
> I hope that's right.

Hrm. So you _do_ want to do an index-only move of A to B, in which case
the suggestion of a "git-mv --cached" seems sensible. Though I'm curious
why you want that. The only workflow I can think of is:

  1. you modify a.c
  2. your boss comes in and tells you to make some unrelated change,
     which involves moving a.c to b.c
  3. You don't want to commit your changes, so you git-mv in the index
     only without involving your dirty working tree file.
  4. You commit the index (which doesn't have your changes from (1)

I think that is sort of a bogus workflow, though, since you will never
have actually compiled or tested the changes in (2). You are much better
to git-stash your current work, fulfill the boss's request, then
unstash.

-Peff

Re: Proposed git mv behavioral change

[Michael Witten]

On 18 Oct 2007, at 11:47:05 PM, Jeff King wrote:

> The only workflow I can think of is:
>
>   1. you modify a.c
>   2. your boss comes in and tells you to make some unrelated change,
>      which involves moving a.c to b.c
>   3. You don't want to commit your changes, so you git-mv in the index
>      only without involving your dirty working tree file.
>   4. You commit the index (which doesn't have your changes from (1)
>
> I think that is sort of a bogus workflow

Ha!

That's exactly what the original poster wanted.

It's not unreasonable.

Re: Proposed git mv behavioral change

[Jeff King]

On Thu, Oct 18, 2007 at 11:53:05PM -0400, Michael Witten wrote:

>>   3. You don't want to commit your changes, so you git-mv in the index
>>      only without involving your dirty working tree file.
> That's exactly what the original poster wanted.
>
> It's not unreasonable.

I guess. I really think git-stash is your friend here. But you can still
do step (3) with git-update-index (I'll leave the exact details as an
exercise for the reader).

-Peff

Re: Proposed git mv behavioral change

[Ari Entlich]

On Thu, 2007-10-18 at 23:47 -0400, Jeff King wrote: 
> On Thu, Oct 18, 2007 at 11:40:47PM -0400, Michael Witten wrote:
> 
> > Anyway, succinctly:
> >
> >> What you want to happen is the following:
> >> 	
> >> 	git show HEAD:A.txt > path/B.txt
> >> 	git add path/B.txt
> >> 	mv A.txt B.txt
> >> 	git rm A.txt

The one above is not only "worse" than the one below, it's wrong. See
Wincent's message and my response to it.

> >
> > Better:
> >
> >>  	mv A.txt path/B.txt
> >> 	Point the index entry for A.txt to path/B.txt
> >
> > I hope that's right.
> 
> Hrm. So you _do_ want to do an index-only move of A to B, in which case
> the suggestion of a "git-mv --cached" seems sensible. Though I'm curious
> why you want that.

Well, I also want to move the working directory file so that the index
and the working directory still match up and so that their differences
are preserved. Therefore, --cached isn't quite right (see Shawn's
messages and my response to them).

> The only workflow I can think of is: 
> 1. you modify a.c
>   2. your boss comes in and tells you to make some unrelated change,
>      which involves moving a.c to b.c
>   3. You don't want to commit your changes, so you git-mv in the index
>      only without involving your dirty working tree file.
>   4. You commit the index (which doesn't have your changes from (1)
> 
> I think that is sort of a bogus workflow, though, since you will never
> have actually compiled or tested the changes in (2). You are much better
> to git-stash your current work, fulfill the boss's request, then
> unstash.

Hmm, that's an interesting point. Are you talking about situations in
which your changes after (1) leave the program in an uncompilable state?
In this situation, I could imagine git stash being a better solution. In
my situation, however, the project was compilable. The reason I didn't
want to commit the changes was because I wasn't entirely satisfied with
how I implemented the change I was trying to make; I thought there might
be a better way to do it. I wanted to get some comparatively
straightforward changes out of the way before I tackled it.

On some level, the reason I want this change isn't entirely because it's
preventing me from doing something I want to do. I've come to really
like git and how the index can be used to separate all of the changes
you have made from the changes that you want to commit in individual
commits. I've come to really like how much access to the lower-level
interfaces git provides. It seems to me that people should support this
simply because it provides more power to the user. If the user wants
their unstaged changes to be staged, they can explicitly do it with git
update-index or some such command.

If the issue is whether this would be the default, that's a completely
separate issue, and one which I don't really have a strong opinion on.
If it were up to me, I'd probably choose to make it the default, but
this might break some people's expectations. I want to keep git
accessible to as many people as possible, not force it into the mold
that I feel is the only correct one. If the functionality is available
but only used when you want it, what's the harm in including it?

I don't really see why you're analyzing the situations in which this
would be used. I think it should be obvious from my descriptions of my
situation that there is an application for this functionality. Unless
you're going to start calling my process bogus (as you did to the one
involving a boss), which I would not appreciate, I'd argue that the same
mentality should be applied to this that is applied to kernel drivers -
if it's useful to even only one person, it should be supported. This
functionality of git wouldn't even be that extreme an example, though -
there's no telling whether somebody might be in a situation like mine
and find that they want to do what I want to do.

But... that's not even the point I was going to make. I think the
questions you should be asking are things like "Does this fit with the
overall architecture?", "Does this or doesn't this provide power and
flexibility to the user?", etc. Is git being made for "the 80%" that use
Subversion and need to be supported by a cushy GUI, or "the 20%" that
thrive on power and are in touch with future trends?

So... yeah. Sorry, I know this got to sounding incredibly
propagandistic, but this is what propaganda's for, right? :)

Sorry if anything I've said here sounded confrontational; that was not
not my intent at all. I'm just raising some points I think are
important. I don't want any flame wars...

On Thu, 2007-10-18 at 23:58 -0400, Jeff King wrote:
> On Thu, Oct 18, 2007 at 11:53:05PM -0400, Michael Witten wrote:
> > It's not unreasonable.
> 
> I guess. I really think git-stash is your friend here. But you can
> still do step (3) with git-update-index (I'll leave the exact details
> as an exercise for the reader).

See my response to Wincent's message for my opinion on using git stash.
I seems to me that using it is more of a workaround than a fix.

Thanks,
	Ari

Re: Proposed git mv behavioral change

[Jeff King]

On Sat, Oct 20, 2007 at 01:55:48AM -0400, Ari Entlich wrote:

> Well, I also want to move the working directory file so that the index
> and the working directory still match up and so that their differences
> are preserved. Therefore, --cached isn't quite right (see Shawn's
> messages and my response to them).

Ah, OK. I understand now. So what you would want could be accomplished
with something like:

  mv A B
  (git-ls-files -s | sed s/A$/B/;
   echo 0 0000000000000000000000000000000000000000	A) \
  | git-update-index --index-info

IOW:
  1. move working tree A to B
  2. create staged entry B, same as staged entry A
  3. remove staged entry A

> Hmm, that's an interesting point. Are you talking about situations in
> which your changes after (1) leave the program in an uncompilable state?

No, I meant more that the state you have staged will never have actually
existed in your working tree, so you could not possibly have tested it.
However, it may be that the changes you are trying to avoid staging are
trivial, and you're willing to accept that. And git shouldn't stand in
your way.

And it sounds like you don't necessarily want to make a _commit_ out of
this, you just want to keep working and not have git-mv munge your
staged state.

> I don't really see why you're analyzing the situations in which this
> would be used. I think it should be obvious from my descriptions of my

Because I was having trouble understanding what Michael was trying to
accomplish with his "here are two methods, and one is better" since they
seemed to do different things. But that is perhaps my fault for joining
the thread in the middle. I may have simply caused more confusion by
getting involved. :)

Now I think I understand exactly what you're trying to accomplish,
and I agree that it makes sense.  Shawn is right that "git-mv --cached"
doesn't do what you want, since to be consistent with other tools it
wouldn't touch the working tree (though I wonder if you would be
satisfied with "git-mv --cached A B; mv A B"). I think at this point the
best way forward is to produce a patch and try to solicit comments (in
particular, which behaviors should be supported (current, --cached, or
what you are proposing) and which should be the default).

> But... that's not even the point I was going to make. I think the
> questions you should be asking are things like "Does this fit with the
> overall architecture?", "Does this or doesn't this provide power and
> flexibility to the user?", etc. Is git being made for "the 80%" that use

There seems to be a strong sentiment among the various gits that you
shouldn't build in flexibility for the sake of flexibility if there's no
good use for it. I tend to be less of that sentiment than some others
here, but I think when proposing a feature it is still worth saying "and
here's why it's useful."

> Sorry if anything I've said here sounded confrontational; that was not
> not my intent at all. I'm just raising some points I think are
> important. I don't want any flame wars...

I think you stated your position fine. And we've had enough flame wars
around here lately to last us a while.

-Peff

Re: Proposed git mv behavioral change

[Michael Witten]

On 20 Oct 2007, at 2:24:00 AM, Jeff King wrote:

> Because I was having trouble understanding what Michael was trying to
> accomplish with his "here are two methods, and one is better" since  
> they
> seemed to do different things. But that is perhaps my fault for  
> joining
> the thread in the middle. I may have simply caused more confusion by
> getting involved. :)

The first method is wrong, because I wasn't considering the fact that
he may have already staged something.

Because I thought both methods achieve the same result, I called the  
latter
"better", because it is more efficient.

Michael Witten
mfwitten

Re: Proposed git mv behavioral change

[Jeff King]

On Sat, Oct 20, 2007 at 02:34:49AM -0400, Michael Witten wrote:

> The first method is wrong, because I wasn't considering the fact that
> he may have already staged something.

Yes, I also didn't consider that, which is what led me to wonder what
his use case could possibly be. And thus we were both confused.

-Peff

Re: Proposed git mv behavioral change

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> No, I meant more that the state you have staged will never have actually
> existed in your working tree, so you could not possibly have tested it.
> However, it may be that the changes you are trying to avoid staging are
> trivial, and you're willing to accept that. And git shouldn't stand in
> your way.
> 
> And it sounds like you don't necessarily want to make a _commit_ out of
> this, you just want to keep working and not have git-mv munge your
> staged state.

Yea, I want the same thing.  :-)

And not because I commit things that weren't tested.  Its because
I do mini code reviews for small hunks at a time.  I stage things
I have looked at, and keep things I'm still unsure about unstaged.
But before I commit I make sure my `git diff` output is empty,
so I know that any tests I do now will match what I have when I
run git-commit.

So lets say I make a change in my Makefile that changes the name
of a source file to be more descriptive of that file's contents.
I've reviewed the Makefile change *and* done the file rename,
but I'm still not done reviewing the stuff in the file.  (Yea,
maybe that should be two different commits, but maybe not, lets
not get into that as it depends very much on context.)

Today I move the file, then unstage the hunks I'm not sure about,
then go back and restage them.  Annoying.  It really disrupts
my workflow.

The killer feature in Git for me is the index.  That sucker saves
me on a daily basis as it lets me organize "this i'm happy with,
this i'm not" while I'm in the middle of making a single new commit.
Today git-mv robs me of it.
 
-- 
Shawn.

Re: Proposed git mv behavioral change

[Jeff King]

On Sat, Oct 20, 2007 at 02:36:28AM -0400, Shawn O. Pearce wrote:

> So lets say I make a change in my Makefile that changes the name
> of a source file to be more descriptive of that file's contents.
> I've reviewed the Makefile change *and* done the file rename,
> but I'm still not done reviewing the stuff in the file.  (Yea,
> maybe that should be two different commits, but maybe not, lets
> not get into that as it depends very much on context.)

Right. So the exact state you have in your index never actually existed
in your working tree. But that's OK if:
  - the changes are trivial and obviously correct
  - you're not actually planning on _committing_ that, you just want to
    build the commit using the index
And in those cases, git-stash is either, respectively, overkill or
totally useless.

Please ignore everything I said on this subject before today...I
obviously hadn't figured out what was going on.

-Peff

Re: Proposed git mv behavioral change

[Karl Hasselström]

On 2007-10-20 02:40:03 -0400, Jeff King wrote:

> Right. So the exact state you have in your index never actually
> existed in your working tree. But that's OK if:
>   - the changes are trivial and obviously correct
>   - you're not actually planning on _committing_ that, you just want
>     to build the commit using the index

Or

    - you plan to revisit the commit at a later time and verify that
      it's OK

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: Proposed git mv behavioral change

[Michael Witten]

On 20 Oct 2007, at 2:36:28 AM, Shawn O. Pearce wrote:

> Today I move the file, then unstage the hunks I'm not sure about,
> then go back and restage them.  Annoying.  It really disrupts
> my workflow.

I know it's against policy, but the proposed change should be set
as the default at some point, in my opinion.

Perhaps when the -u flagged is not used, there can be a warning that
states -u will become the default at a certain time.

In fact, -u speaks "update" to me, and I would expect it to signal
the current behavior.

I have a feeling that my suggestion will not go far,
but I also think that backwards compatibility can
overstay its welcome.

Re: Proposed git mv behavioral change

[Shawn O. Pearce]

Michael Witten <mfwitten@MIT.EDU> wrote:
> 
> On 20 Oct 2007, at 2:36:28 AM, Shawn O. Pearce wrote:
> 
> >Today I move the file, then unstage the hunks I'm not sure about,
> >then go back and restage them.  Annoying.  It really disrupts
> >my workflow.
> 
> I know it's against policy, but the proposed change should be set
> as the default at some point, in my opinion.

Its not so much policy as a timing issue.

Git 1.5.4 shouldn't have major distruptions to end-users in terms
of changing existing behavior to be different than in 1.5.3.
Especially default behavior.

Git 1.6.0 we're a little bit more willing to change default
behavior as it is a full major release.  This is probably quite a
bit off still, unless we started to accumulate a lot of really good
changes that required breaking something in the user interface.
That's what happened with the 1.4.4 series vs. 1.5.0.  Right now
I don't see that happening anytime soon.

> I have a feeling that my suggestion will not go far,
> but I also think that backwards compatibility can
> overstay its welcome.

I agree.

-- 
Shawn.

Re: Proposed git mv behavioral change

[Wincent Colaiuta]

El 20/10/2007, a las 8:45, Michael Witten escribió:

> On 20 Oct 2007, at 2:36:28 AM, Shawn O. Pearce wrote:
>
>> Today I move the file, then unstage the hunks I'm not sure about,
>> then go back and restage them.  Annoying.  It really disrupts
>> my workflow.
>
> I know it's against policy, but the proposed change should be set
> as the default at some point, in my opinion.

I think the issue here is that git-mv as it is currently implemented  
really conflates two things:

1. Renaming a file in the traditional "mv" sense
2. Staging the entire contents of the file in the index, ready or not

So it's kind of like the command were called git-mv-and-add or git- 
rename-and-add. And given that the index as a staging area is such a  
central content in Git, users often want to have more control over  
what gets added to the index than that; ie. "I really just wanted to  
rename the file, and leave the staging of modifications to the  
content up to me".

Cheers,
Wincent

Re: Proposed git mv behavioral change

[Karl Hasselström]

On 2007-10-20 13:15:19 +0200, Wincent Colaiuta wrote:

> I think the issue here is that git-mv as it is currently implemented
> really conflates two things:
>
> 1. Renaming a file in the traditional "mv" sense
> 2. Staging the entire contents of the file in the index, ready or not
>
> So it's kind of like the command were called git-mv-and-add or git-
> rename-and-add. And given that the index as a staging area is such a
> central content in Git, users often want to have more control over
> what gets added to the index than that; ie. "I really just wanted to
> rename the file, and leave the staging of modifications to the
> content up to me".

I've come to that conclusion too while reading this thread. It would
make more sense for git-mv to, as others have already suggested, move
the file in the worktree and move the file in the index but _not_ add
workdir updates to the index. git-mv --cached would do only the index,
and not touch the worktree.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: Proposed git mv behavioral change

[Ari Entlich]

On Thu, 2007-10-18 at 21:54 -0400, Shawn O. Pearce wrote:
> Line wrapping email at under 80 columns would be nice. It makes it
> easier to read the message, and more importantly, easier to quote
> a few times during discussion.

Yeah, sorry about that. As I said, the crappy webmail client I was using
was screwing up. I'm using a real email client now.

> I'm somewhat hesistant to change existing behavior, as users may
> be used to it or relying upon it within their scripts. But you
> make an excellent argument about why the current git-mv behavior
> is perhaps less than ideal.

See my response to Wincent's message for what I said about this.

One minor issue (since you talk about programming stuff below) is that,
from my cursory survey of builtin-mv.c, a slight restructuring might be
required for this to work and for the solution to satisfy my coding
standards. This would be because you'd need to change the names of
individual cache entries as opposed to removing one and adding one. This
is a minor issue though, I'm sure I can figure out how to get it to work
somehow.

> Elsewhere in git we use the --cached command line option to mean
> "only make the change in the index". For example the git-apply
> --cached option. You could start a patch that uses --cached to
> trigger the new behavior you propose and see if people are interested
> in changing the default once the feature is working and available
> for experimentation.

>From a later email...
> But I was originally *way* wrong to propose --cached for this usage
> in git-mv. --cached means "apply *ONLY* to the index" and "do *NOT*
> touch the working tree". Here we want to touch the working tree
> in the sense of moving the file. So --cached is not the correct
> option name.

Hmm yeah, that did occur to me after I read your original message...

> --index is used in Git for places were we update *both* the index
> and the working directory (git-apply --index). So actually I should
> have suggested "git-mv --index".  Whoops.

Alright then, I don't know about that particular convention. If this
behavior can't be made default, git mv --index should activate it? I
there anything else that might be more descriptive?

> > I'm willing to look into what changes would need to be made to the
> > code for this change to happen; I'm not asking for someone to do
> > all the work for me. :)
> > 
> > So... Yeah. I'd like to know what people think about this before
> > I put a significant amount of effort into it. After all, we know
> > how lazy programmers are... :)
> 
> See builtin-mv.c around l.264-283. This is where we are removing
> the old names from the index (in memory) and inserting the new
> names. Instead of calling add_file_to_cache() you would want
> to use something like add_cacheinfo() in builtin-update-index.c,
> specifying the old sha1, ce_flags and ce_mode.

Might it be possible to simply get the struct cache_entry for the file
which we want to rename, change its name property (would this involve
xreallocing it?), and change the ce_namelen field of ce_flags?

In any case, I think the ce_flags would need to be changed to reflect
the new name length. Also, it seems that the old ce_mtime, ce_dev,
ce_ino, ce_uid, ce_gid, and ce_size could be used too... ce_ctime would
need to be updated...

> I'm sure Junio could probably give you a better starting point
> than I can, as he's more familiar with this sort of code, but that
> should still get you looking in the right direction and maybe get
> a working implementation together that you can share for discussion.

Yeah, I'd appreciate any help I can get. The people on #git were
invoking the "the code is the documentation" ideology. :)

So... Ping Junio...? I actually haven't seen any messages from him since
I subscribed to this list (I've only gotten about 300 messages so far,
but I'd expect to see a lot from him as he's the maintainer...). Is he
away at the moment or something?

Thanks,
	Ari

Re: Proposed git mv behavioral change

[Shawn O. Pearce]

Ari Entlich <lmage11@twcny.rr.com> wrote:
> On Thu, 2007-10-18 at 21:54 -0400, Shawn O. Pearce wrote:
> > --index is used in Git for places were we update *both* the index
> > and the working directory (git-apply --index). So actually I should
> > have suggested "git-mv --index".  Whoops.
> 
> Alright then, I don't know about that particular convention. If this
> behavior can't be made default, git mv --index should activate it? I
> there anything else that might be more descriptive?

That's always the hard part.  I actually think the current behavior
should be called --index as it does not only the working tree
update but also stages the whole file into the index, which is what
git-apply --index does.

What about just -u for "keep unstaged"?
 
> Might it be possible to simply get the struct cache_entry for the file
> which we want to rename, change its name property (would this involve
> xreallocing it?), and change the ce_namelen field of ce_flags?

Yes.
 
> In any case, I think the ce_flags would need to be changed to reflect
> the new name length. Also, it seems that the old ce_mtime, ce_dev,
> ce_ino, ce_uid, ce_gid, and ce_size could be used too... ce_ctime would
> need to be updated...

Correct.  The ce_size shouldn't change as a result of the rename but
the other properties of a struct stat may.  I'd like to think that
ce_uid and ce_gid wouldn't change as a result of a rename but there's
probably some bastard filesystem somewhere that that won't hold true.

Just call fill_stat_cache_info() on your xrealloc'd entry and pass
it a stat buffer for the file after the rename and you should be
covered here.

> > I'm sure Junio could probably give you a better starting point
> > than I can, as he's more familiar with this sort of code, but that
> > should still get you looking in the right direction and maybe get
> > a working implementation together that you can share for discussion.
> 
> Yeah, I'd appreciate any help I can get. The people on #git were
> invoking the "the code is the documentation" ideology. :)

It really is in this case.  You are talking about hacking on
Git itself.  For the most part we believe in "the code is the
documentation" for the guts as otherwise the documentation gets
out of sync with the code when the code changes.  User level docs
are different as the user interface needs to stay stable.

> So... Ping Junio...? I actually haven't seen any messages from him since
> I subscribed to this list (I've only gotten about 300 messages so far,
> but I'd expect to see a lot from him as he's the maintainer...). Is he
> away at the moment or something?

Yes.  We run like 100 messages a day so I guess you've only joined
in the past few days.  Welcome to the list.  :-)

Junio has been away for about 2 weeks now.  He's basically off
on leave as he needed some time to take care of some things.
I'm filling in for him until he returns.

-- 
Shawn.

Re: Proposed git mv behavioral change

[Mike Hommey]

On Sat, Oct 20, 2007 at 02:30:14AM -0400, Shawn O. Pearce wrote:
> Ari Entlich <lmage11@twcny.rr.com> wrote:
> > On Thu, 2007-10-18 at 21:54 -0400, Shawn O. Pearce wrote:
> > > --index is used in Git for places were we update *both* the index
> > > and the working directory (git-apply --index). So actually I should
> > > have suggested "git-mv --index".  Whoops.
> > 
> > Alright then, I don't know about that particular convention. If this
> > behavior can't be made default, git mv --index should activate it? I
> > there anything else that might be more descriptive?
> 
> That's always the hard part.  I actually think the current behavior
> should be called --index as it does not only the working tree
> update but also stages the whole file into the index, which is what
> git-apply --index does.
> 
> What about just -u for "keep unstaged"?

Why not --staged ? It would be more meaningful than the --cached in some
other commands.

Mike

Re: Proposed git mv behavioral change

[Jeff King]

On Sat, Oct 20, 2007 at 01:55:58AM -0400, Ari Entlich wrote:

> Might it be possible to simply get the struct cache_entry for the file
> which we want to rename, change its name property (would this involve
> xreallocing it?), and change the ce_namelen field of ce_flags?

The sort order of entries in the cache is important, so you would have
to move the entry, as well.

-Peff

Re: Proposed git mv behavioral change

[lmage11]

Uh... wow. Ok, this thread has really taken off. Sorry, but I can't be
involved with it right now, I'll get back into it sometime tomorrow. Try
not to get too far ahead of me! :)

Also sorry about the crazy line breaking, I'm using a crappy webmail
client at the moment...

Thanks,
   Ari

Re: Proposed git mv behavioral change

[Ari Entlich]

On Fri, 2007-10-19 at 13:33 +0200, Wincent Colaiuta wrote:
> El 19/10/2007, a las 4:29, Michael Witten escribió:
> > Ah. Basically my 'pseudo-code' is correct, but redundant.
> 
> If I understood the original poster's proposal then I don't think your
> code does what he asked for:
> 
> > What you want to happen is the following:
> > 
> > git show HEAD:A.txt > path/B.txt
> > git add path/B.txt
> > mv A.txt B.txt
> > git rm A.txt
> >
> > Is this correct?
> 
> Here you're copying the content of A.txt as it was in the last (HEAD)
> commit, but from what the poster said he wants the content of A.txt as
> it is staged in the index (that is, there may be staged but uncomitted
> changes).

You took the words right out of my mouth! :)

Yeah, I noticed the problem with using HEAD, but the other problem is
that this would change the contents of the file in the working directory
file, which I don't want. Thus, putting the contents of the file as it
is in the index into the working directory wouldn't be correct either.
In addition, I'm not quite sure where that "mv A.txt B.txt" came from,
since we're supposed to be moving A.txt to path/B.txt...

> > Better:
> >
> > > mv A.txt path/B.txt
> > > Point the index entry for A.txt to path/B.txt
> 
> Yes, that is basically what he was asking for, as I read it.

Yep! I was going to respond to your (Michael's) original message saying
exactly that. :)

> El 19/10/2007, a las 5:47, Jeff King escribió:
> > Hrm. So you _do_ want to do an index-only move of A to B, in which 
> > case the suggestion of a "git-mv --cached" seems sensible. Though 
> > I'm curious why you want that.
> 
> I agree that git-stash can be used in this workflow but I can also 
> imagine cases where the proposed "git-mv --cached" might be a bit 
> nicer.

As Shawn said, --cached wouldn't be entirely accurate as the file in the
working directory is being moved as well.

git-stash has been suggested to me numerous times, but I really feel
that there's no need to use it in this case - if the git mv command gave
adequate control to the user, it would be unnecessary.

> I'm thinking of occasions where you just want to do something 
> like:
> 
> git mv --cached foo bar
> git add --interactive bar

I think it would be the other way around, since the only time this
change would effect anything is when there are changes still waiting to
be staged.

Are you talking about REALLY only changing the index? I can't think of
why you'd want to do this either... After all, wouldn't there be no bar
file to do git add --interactive on? In addition, I don't think giving
--interactive a filename is meaningful...

> I'm not sure the proposed "--cached" switch should ever be the 
> default -- would need to ponder that one -- but I do think the switch 
> would be a nice addition.

Yeah, that is one thing I was wondering. It would break compatibility,
but would it be enough to put a note about that in the announcements for
a release? Could you make the change at a release when the interface
isn't guaranteed to be the same, or is this practice only done with
libraries?

It might be interesting to do some sort of survey of whether people
depend on this behavior. It seems pretty inconsistent with how git works
otherwise, and I'd be surprised if a lot of people expect it (kinda like
the Spanish Inquisition :-P).

Thanks,
	Ari

Re: Proposed git mv behavioral change

[Jeff King]

On Sat, Oct 20, 2007 at 01:55:53AM -0400, Ari Entlich wrote:

> git-stash has been suggested to me numerous times, but I really feel
> that there's no need to use it in this case - if the git mv command gave
> adequate control to the user, it would be unnecessary.

Yes, now that I understand what you're doing, git-stash is probably
overkill. It's great if you really want to put aside your current state
and do some other, totally unrelated work.

But in this case, you are talking about tools for managing the index
correctly, which is a totally different use case.

> > git mv --cached foo bar
> Are you talking about REALLY only changing the index? I can't think of
> why you'd want to do this either... After all, wouldn't there be no bar
> file to do git add --interactive on? In addition, I don't think giving
> --interactive a filename is meaningful...

Originally I had thought git-mv --cached would be useful (and a nice
orthogonal design), but it really _does_ seem useless, since it's
awkward to get your working tree changes into the index without actually
moving the working tree file.

> It might be interesting to do some sort of survey of whether people
> depend on this behavior. It seems pretty inconsistent with how git works
> otherwise, and I'd be surprised if a lot of people expect it (kinda like
> the Spanish Inquisition :-P).

The best way to get comments is to make a patch.

As for inconsitency of git-mv, it has always been a bit of an odd duck.
I'm fairly certain neither Linus nor Junio actually use it, preferring
instead to make all changes to the working tree and then update the
index manually. I even seem to recall Linus arguing that git-mv was
stupid and unnecessary a long time ago, but I could be wrong.

-Peff

Re: Proposed git mv behavioral change

[Wincent Colaiuta]

El 20/10/2007, a las 7:55, Ari Entlich escribió:

>> I'm thinking of occasions where you just want to do something
>> like:
>>
>> git mv --cached foo bar
>> git add --interactive bar
>
> I think it would be the other way around, since the only time this
> change would effect anything is when there are changes still  
> waiting to
> be staged.

Well, my point was that sometimes you want to rename a dirty file  
*right now* without having your modifications staged in the index  
yet, and only later go ahead and stage the hunks (or the whole file)  
when they're ready.

Basically, without this feature you have to manually unstage the  
stuff that you don't want staged:

[hack on dirty foo]
git mv foo bar
[oops... you just staged unfinished changes]
git reset HEAD bar
[hack until bar is ready]
git add bar

So in order to unstage the stuff that wasn't ready you unstaged the  
whole file and had to re-add it later, in which case what was the  
point of using "git-mv" in the first place? You may as well have just  
done:

[hack on dirty foo]
mv foo bar
[hack until bar is ready]
git rm foo
git add bar

The other alternative is to use git-stash:

[hack on dirty foo]
git stash
git mv foo bar
git stash apply
[hack until bar is ready]
git add bar

So, yes, you can do this with git-stash. It just means that "git-mv -- 
cached" would be a convenient short-cut.

> Are you talking about REALLY only changing the index?

No, sorry, I didn't make that clear. I meant that "git mv --cached  
foo bar" would have the following effect (which if I understand it  
correctly is basically what you proposed in your first email):

1. Copying "foo" directly to "bar" (even if dirty).

2. When adding "bar" to the the index add the blob corresponding to  
"foo" as it is staged in the index (or at the HEAD if there are no  
staged changes).

3. Removing the old file "foo".

The actual mechanics of this don't matter; those are just the  
perceived effects. You could get exactly the same perceived effects  
by doing it this way:

1. Creating a new file (or overwriting an existing one) named "bar",  
whose contents would match those of the file "foo" not as it appears  
in the working tree but as it is staged in the index.

2. If "foo" has unstaged changes, they should be applied to "bar" as  
well (but not staged).

3. Removing the old file "foo".

> In addition, I don't think giving
> --interactive a filename is meaningful...

Whoops. I think I just inadvertently proposed a feature... my most  
common use of "git-add --interactive" is when I want to stage only  
specific hunks of a particular file, and so instead of typing "git  
add bar" I want to type "git add -i bar" and have it jump straight to  
the "patch" subcommand (5) for that file. Would anyone else find this  
useful?

Cheers,
Wincent

Re: Proposed git mv behavioral change

[Jeff King]

On Sat, Oct 20, 2007 at 01:02:32PM +0200, Wincent Colaiuta wrote:

> Whoops. I think I just inadvertently proposed a feature... my most common 
> use of "git-add --interactive" is when I want to stage only specific hunks 
> of a particular file, and so instead of typing "git add bar" I want to type 
> "git add -i bar" and have it jump straight to the "patch" subcommand (5) for 
> that file. Would anyone else find this useful?

Yes, my only use of git-add --interactive is to stage particular patches
from individual files. So I would find that feature useful.

-Peff