patchadd: make "add -u" update full tree without pathspec

When -u was introduced in dfdac5d (git-add -u: match the index with working tree., 2007-04-20), "add -u" (without pathspec) added everything. Shortly after, 2ed2c22 (git-add -u paths... now works from subdirectory, 2007-08-16) broke it while fixing something related.

This makes -u (and -A) inconsistent with some other options, namely -p. It's been four years since the unintentional breakage and people are probably used to "git add -u" updating only current directory. Perhaps it's time to bring the original behavior back? Current behavior can always be achieved with "git add -u ."

Migration plan:

I'm bad at this. Can we start with a patch that warns users to do "git add -u ." when they do "git add -u"? Hopefully they would have their fingers retraied by the time the behavior is changed in 1.8.0.

PS. What about -A?

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 2011/2/7 Sebastian Pipping <webmaster@hartwork.org>:
 >> git add -u was tree-wide when it was introduced in dfdac5d (git-add
 >> -u: match the index with working tree., 2007-04-20), but 2ed2c22
 >> (git-add -u paths... now works from subdirectory, 2007-08-16) broke it
 >> while fixing something related.
 >
 > So my memory didn't fool me.  Thanks for digging this out.
 >
 > Can we have tree-wide "git add -u" back, please?

 Yup yup I like it too (and wanted the original behavior sometimes, even
 though I didn't know it was original behavior).

 Pulling Junio in for -A. It seems closely related to -u. In fact I revert
 one line from 1e5f764 (builtin-add.c: optimize -A option and "git add ."
 - 2008-07-22) but not fully understand why it was changed.

 builtin/add.c         |    7 +++++--
 t/t2200-add-update.sh |   13 +++++++------
 2 files changed, 12 insertions(+), 8 deletions(-)

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

As long as the command takes pathspecs, it should never be tree-wide. Making it tree-wide when there is no pathspec is even worse.

If "add -p" does not limit operation to the current directory, probably that is the inconsistency bug to be fixed.

On Mon, Feb 7, 2011 at 12:58 PM, Junio C Hamano <gitster@pobox.com> wrote:

git log -p and diff family all can take pathspecs. All default to tree-wide without pathspecs. This is what I'm doing all the time:

git diff # checking, ok, looks good git add -u # ack, need to come to root dir first

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Not to mention "git status", which shows everything in the tree when given no args, but will only show changes underneath the cwd if given "." as an argument.

I agree that the "default to root of git dir when given no args" behavior seems both more convenient -- it's annoying to specify the root of the tree explicitly, but very easy to specify the cwd explicitly (".") -- and more intuitive -- If I want "everything underneath cwd", I'll naturually specify "." as an argument; if I give no args, I almost certainly want the whole tree, no matter where I happen to be in it.

-Miles

On Feb 6, 2011, at 10:14 PM, Nguyen Thai Ngoc Duy wrote:

The only reason I'm not routinely bitten by this is that I'm generally always at the root of the tree. But I certainly *think* about it as an operation that's tree-wide, not local. If I meant to limit the scope, I'd specify a path.

Josh

[ Your mailer did something strange with my name and email ]

Junio C Hamano <gitster@pobox.com> writes:

This is against the common use in Git. As I mentionned in the other thread, pretty much any git command taking an optional pathspec is still tree-wide when called without argument. "git add -u" is the exception.

Add "git log", "git status", "git commit", "git show", "git diff", and a few others I've missed to the list of "exceptions" ...

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Perhaps in this order:

Step 1, as soon as possible:

 * Introduce "static int make_update_global;" file scope static to
   builtin/add.c and default to 0;

 * Introduce "add.make_update_global" configuration variable, and toggle
   the above variable when it is explicitly given; also record the fact
   that you actually saw this variable in the config parser regardless of
   the value that is given;

 * Document the configuration variable as a new feature, without
   indicating that that will be the new default in the future, but
   strongly recommending that existing scripts should be updated and new
   scripts should be written the variable in mind---namely, their use of
   "add -u" should use "." if they are relying on the current "limit to
   cwd" behaviour.

 * Ship this as 1.7.X and see how well the new feature is accepted in the
   wider user community.  If the feature is not widely used, there is no
   point in proceeding further.

Step 2, two cycles (i.e. 6-8 months) before 1.8.0, provided if the previous step is proven positive:

 * When "git add -u" is run without any pathspec, and when the user does
   not have the configuration variable, warn loudly that the default
   behaviour will change in 1.8.0, and tell the users that the current
   behaviour is still available by setting the configuration variable.

Step 3, at 1.8.0 and probably one or two more cycles:

 * When "git add -u" is run without any pathspec, and when the user does
   not have the configuration variable, warn loudly that the default
   behaviour _has_ changed in 1.8.0, to help people who migrated from
   early 1.7.X before Step 2 directly to 1.8.X series.

Step 4, 3 cycles after 1.8.0:

 * Drop the warning.

I personally think it should change exactly the same way as "-u" for consistency, but I had the impression that the list concensus was to keep the current behaviour.

Has anybody been keeping tallies on the discussion around

  http://thread.gmane.org/gmane.comp.version-control.git/166135/focus=166457

We seem to have lost track. All I recall was that I changed my mind and migrated from the third camp (the command defaults to whichever somebody thought would be more convenient, lacking consistency across command set) to "it would be more convenient to default to full-tree when there is no pathspec, as limiting to cwd with '.' is easier" camp myself, but the discussion is not about my personal preference but about seeing if we have an overwhelming majority consensus, so...

Junio C Hamano <gitster@pobox.com> writes:

I'd even say "error out", since the old behavior is still easily available with "git add -u ." (which should be mentionned in the warning/error message).

Same here. I care less since I use "add -A" less, but both commands should really behave the same since they're very similar.

On Sun, Feb 27, 2011 at 6:43 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

You wouldn't want scripts to break this way. Warnings can give script writers some time to update (assume that scripts are installed system wide, not in $HOME).

Heya,

On Sun, Feb 27, 2011 at 11:46, Junio C Hamano <gitster@pobox.com> wrote:

I'm confused, I thought we were doing this for consistency reasons? That would benefit mostly new users, right? New users, which are unlikely to notice this new feature, or report to this list that they find it useful?

On Sun, Feb 27, 2011 at 8:35 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:

I think the purpose is to see if this benefits current user base first, which is large enough. If it beats the user base hard, it's not worth adding even for consistency. Unfortunately not all of us users reads release notes. I don't know how we know it's widely used. Putting something like this in docs?

"This is experimental. If you use it and like it, raise your voice in git@ver"

On Sun, Feb 27, 2011 at 5:46 PM, Junio C Hamano <gitster@pobox.com> wrote:

Ermm.. compat.make_update_global, with the intent that the config will be dropped in future (1.9.0 maybe)?

There's a problem. I use git on many machines. Some will have this config enabled, some will not (yet). Perhaps a third option, which will print something when "git add -u" is issued as a reminder? I don't know. What I need is to notice old behavior of "git add -u" before it's too late.

Step 5 (1.9.0 or later):

 * Drop compat.make_update_global.

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

As you haven't yet proven that this "new feature" is even useful to help new people nor existing users at this step, you cannot claim "we plan to drop this in the future", hence naming it "compat.*" is a no-go. During this step, we can not even say "we plan to make this the default"; we would confuse the users otherwise (it is fine to say "we might make this the default some day").

Even if we indeed end up proceeding to step 2, I don't see a point in planning to drop the support from the beginning. We might end up doing so, but we can decide when that becomes necessary, and that would be long after the tree-wide default proves a reasonable one, and preferably after seeing a new person or two raise "what's the point of making 'add -u' restricted to cwd? we have too many options and this can go" on the list.

Then we would start deprecating the config, giving a warning when people who still rely on their "add.make_update_global = false" say "add -u" without pathspec in a few cycles, and then finally drop it at a version bump boundary.

Such a warning would not help you on a machine that does not even have git with Step 1 change.

What you conceive as a problem is just a reminder that any incompatible change you plan to add will have pain involved. On two machines, one with a new feature and the other without the new feature, you would have to work differently _or_ you would train yourself to use both versions in a compatible way (e.g. when you mean tree-wide, you would cdup, and when you mean cwd, you would explicitly say ".", from the command line). That is not limited to this particular feature but any incompatible change, no?

On Mon, Feb 28, 2011 at 2:39 AM, Junio C Hamano <gitster@pobox.com> wrote:

No. But I hoped it would help me somehow, or at least remind me to update git on the machines I touch.

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Perhaps the migration plan is not helpful enough? If that is the case we would need to rethink it to be even less impactful.

I already do not like the possibility of potential double deprecation myself, one to flip the default and then possibly another to drop the support of the traditional behavour, but anything I've seen so far would hurt the end users more than that plan.

On Mon, Feb 28, 2011 at 1:56 PM, Junio C Hamano <gitster@pobox.com> wrote:

But how many people may be impacted this way? If it's few, probably not worth the headaches. I want to believe the number is few, but I think anybody who ssh to some machine may have the same problem because they may not control what's in that machine.

I can only think of another way, ugly though: add a new command name, say "nad", that goes with new behavior, "add" remains old school. This way if I mistakenly type 'nad' on unsupported git, it refuses and I'm safe. Whether the new name is temporary (until the behavior flip in "add") or permanent is another ugly matter to think about.

Or accept that evolution is painful and go with current plan, which I'm OK too.

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

So even though you raised a concern on possible pains during the migration, you think that the migration plan outlined would be the least bad one?

On Tue, Mar 1, 2011 at 8:46 PM, Junio C Hamano <gitster@pobox.com> wrote:

I only see two ways. The 'new command' approach only delays the transition until a later (user decided) point. There's no guarantee that I (user) would make a good decision to switch. Plus the outlined plan looks simpler. Yes, it seems better.

Junio C Hamano <gitster@pobox.com> writes:

I think promoting "git add -u ." more than a configuration option would reduce the pain.

As a user, if I get used to typing "git add -u ." instead of "git add -u", I get the current behavior regardless of the version of Git, without a warning. Later, when all the machines I word on support the tree-wide "git add -u" (either 1.7.x + some configuration or 1.8.y), I'll use it as a new feature.

So, a warning like

  warning: the behavior of "git add -u" without pathspec will change in
  Git 1.8.0. To keep the current behavior, use this instead:
  
      git add -u .
  
  + explanations about the config options as already discussed here

would be fine.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Yeah, I tend to agree.

Once your users (you as a script writer) have an option to set the configuration to participate in the tree-wide party early, you would need to update your scripts immediately so that they don't break on them; so the introduction of the configuration becomes a flag-day event. Hmph...

Yeah, I think you convinced me that an elaborate configuration wouldn't help us at all. We just keep warning in 1.7.x series when "add -u" didn't see any pathspec, and flip the default at 1.8.0

Simpler and cleaner ;-)

Junio C Hamano <gitster@pobox.com> writes:

But "update" should mean "replace git add -u with git add -u .", which is the portable way to do the same.

The introduction of the config variable is a non-event if you already use the portable . notation.

I think is still makes sense to have a config variable, so that people who want the new behavior can get it ASAP. Right after 1.8 is out, I'll still have machines where I'm too lazy to install a brand new Git, and I'll want to get the 1.8 goodness for free ;-).

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

[...]

So it would be the opposite of compat.*; for example `future.wholeTree` config variable? ;-)

On Tue, Mar 01, 2011 at 07:51:38PM +0100, Matthieu Moy wrote:

Right. Once this happens, you can never say "git add -u" again portably. You can't rely on the old behavior, because it's changing. You can't rely on the new behavior, because you might be using an old version. So as a script you _must_ say "git add -u .", and if you want top-level behavior and are not at the top-level, you must cd to the toplevel and "add .". Which sounds onerous, but it is what scripts have to do already with the current behavior.

Agreed. I also think there should be a setting to keep the current behavior. I don't want to use it, but given that configuration does not introduce any existing portability issues, we can help people who really liked the old behavior. We are inconveniencing them by changing the default, but it seems doubly mean to leave them with no way of restoring it short of typing extra characters on every invocation.

-Peff

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

I think you got this part wrong. Until now, there was no "portability" to worry about when using "git add -u". "git add -u" used to be _guaranteed_ to be relative to the cwd, with or without ".". Our update will _break_ that expectation and suddenly make "." "the portable notation".

In other words, it is far from a non-event. We cannot say "'add -u .' is the portable way to write it anyway, so you should have been writing that from day one" as an excuse for breaking their scripts.

We still have to say "We are sorry, we will be breaking your scripts. Please add an extra dot at the end". I am Ok saying that, but we should admit that we are knowingly breaking things.

Junio C Hamano <gitster@pobox.com> writes:

There is a "if" in my sentence, and I do claim that if you verify the condition behind the "if", it is a non-event.

I did not claim that the whole change was a non-event, just that the introduction of a config variable was.

Absolutely. My claim is just that adding the extra dot is the way to fix script wrt the future change, or at least a better way than relying on a config variable.