* [PATCH 0/4] Deprecate "not allow as-is commit with i-t-a entries"
@ 2012-02-06 10:57 Nguyễn Thái Ngọc Duy
2012-02-06 10:57 ` [PATCH 1/4] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
By the end of this series "git commit -m foo" will proceed regardless
intent-to-add (aka "git add -N") entries. commit.ignoreIntentToAdd is
used as transitiono config key.
The plan is in 2/4. I set 1.8.0 as the first deprecation date, but of
course it's just a suggestion. The second deprecation date might be
1.8.5, long enough for users to adapt to the new behavior.
..or we switch back half way because we find current behavior does
make more sense.
Nguyễn Thái Ngọc Duy (4):
cache-tree: update API to take abitrary flags
commit: introduce a config key to allow as-is commit with i-t-a
entries
commit: turn commit.ignoreIntentToAdd to true by default
commit: remove commit.ignoreIntentToAdd, assume it's always true
Documentation/git-add.txt | 12 ++++++++++--
builtin/commit.c | 9 ++++++---
cache-tree.c | 35 +++++++++++++++++------------------
cache-tree.h | 5 ++++-
merge-recursive.c | 2 +-
t/t2203-add-intent.sh | 21 ++++++++++++++++++++-
test-dump-cache-tree.c | 2 +-
7 files changed, 59 insertions(+), 27 deletions(-)
--
1.7.8.36.g69ee2
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/4] cache-tree: update API to take abitrary flags
2012-02-06 10:57 [PATCH 0/4] Deprecate "not allow as-is commit with i-t-a entries" Nguyễn Thái Ngọc Duy
@ 2012-02-06 10:57 ` Nguyễn Thái Ngọc Duy
2012-02-06 19:51 ` Junio C Hamano
2012-02-06 10:57 ` [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries Nguyễn Thái Ngọc Duy
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/commit.c | 4 ++--
cache-tree.c | 27 ++++++++++++---------------
cache-tree.h | 4 +++-
merge-recursive.c | 2 +-
test-dump-cache-tree.c | 2 +-
5 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index eba1377..bf42bb3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -400,7 +400,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
fd = hold_locked_index(&index_lock, 1);
add_files_to_cache(also ? prefix : NULL, pathspec, 0);
refresh_cache_or_die(refresh_flags);
- update_main_cache_tree(1);
+ update_main_cache_tree(WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(&index_lock))
die(_("unable to write new_index file"));
@@ -421,7 +421,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
if (active_cache_changed) {
- update_main_cache_tree(1);
+ update_main_cache_tree(WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(&index_lock))
die(_("unable to write new_index file"));
diff --git a/cache-tree.c b/cache-tree.c
index 8de3959..16355d6 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -150,9 +150,10 @@ void cache_tree_invalidate_path(struct cache_tree *it, const char *path)
}
static int verify_cache(struct cache_entry **cache,
- int entries, int silent)
+ int entries, int flags)
{
int i, funny;
+ int silent = flags & WRITE_TREE_SILENT;
/* Verify that the tree is merged */
funny = 0;
@@ -241,10 +242,11 @@ static int update_one(struct cache_tree *it,
int entries,
const char *base,
int baselen,
- int missing_ok,
- int dryrun)
+ int flags)
{
struct strbuf buffer;
+ int missing_ok = flags & WRITE_TREE_MISSING_OK;
+ int dryrun = flags & WRITE_TREE_DRY_RUN;
int i;
if (0 <= it->entry_count && has_sha1_file(it->sha1))
@@ -288,8 +290,7 @@ static int update_one(struct cache_tree *it,
cache + i, entries - i,
path,
baselen + sublen + 1,
- missing_ok,
- dryrun);
+ flags);
if (subcnt < 0)
return subcnt;
i += subcnt - 1;
@@ -371,15 +372,13 @@ static int update_one(struct cache_tree *it,
int cache_tree_update(struct cache_tree *it,
struct cache_entry **cache,
int entries,
- int missing_ok,
- int dryrun,
- int silent)
+ int flags)
{
int i;
- i = verify_cache(cache, entries, silent);
+ i = verify_cache(cache, entries, flags);
if (i)
return i;
- i = update_one(it, cache, entries, "", 0, missing_ok, dryrun);
+ i = update_one(it, cache, entries, "", 0, flags);
if (i < 0)
return i;
return 0;
@@ -572,11 +571,9 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
was_valid = cache_tree_fully_valid(active_cache_tree);
if (!was_valid) {
- int missing_ok = flags & WRITE_TREE_MISSING_OK;
-
if (cache_tree_update(active_cache_tree,
active_cache, active_nr,
- missing_ok, 0, 0) < 0)
+ flags) < 0)
return WRITE_TREE_UNMERGED_INDEX;
if (0 <= newfd) {
if (!write_cache(newfd, active_cache, active_nr) &&
@@ -672,10 +669,10 @@ int cache_tree_matches_traversal(struct cache_tree *root,
return 0;
}
-int update_main_cache_tree (int silent)
+int update_main_cache_tree (int flags)
{
if (!the_index.cache_tree)
the_index.cache_tree = cache_tree();
return cache_tree_update(the_index.cache_tree,
- the_index.cache, the_index.cache_nr, 0, 0, silent);
+ the_index.cache, the_index.cache_nr, flags);
}
diff --git a/cache-tree.h b/cache-tree.h
index 0ec0b2a..d8cb2e9 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -29,13 +29,15 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);
struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);
int cache_tree_fully_valid(struct cache_tree *);
-int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int, int, int);
+int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int);
int update_main_cache_tree(int);
/* bitmasks to write_cache_as_tree flags */
#define WRITE_TREE_MISSING_OK 1
#define WRITE_TREE_IGNORE_CACHE_TREE 2
+#define WRITE_TREE_DRY_RUN 4
+#define WRITE_TREE_SILENT 8
/* error return codes */
#define WRITE_TREE_UNREADABLE_INDEX (-1)
diff --git a/merge-recursive.c b/merge-recursive.c
index d83cd6c..6479a60 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -264,7 +264,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
if (!cache_tree_fully_valid(active_cache_tree) &&
cache_tree_update(active_cache_tree,
- active_cache, active_nr, 0, 0, 0) < 0)
+ active_cache, active_nr, 0) < 0)
die("error building trees");
result = lookup_tree(active_cache_tree->sha1);
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index e6c2923..a6ffdf3 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -59,6 +59,6 @@ int main(int ac, char **av)
struct cache_tree *another = cache_tree();
if (read_cache() < 0)
die("unable to read index file");
- cache_tree_update(another, active_cache, active_nr, 0, 1, 0);
+ cache_tree_update(another, active_cache, active_nr, WRITE_TREE_DRY_RUN);
return dump_cache_tree(active_cache_tree, another, "");
}
--
1.7.8.36.g69ee2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-06 10:57 [PATCH 0/4] Deprecate "not allow as-is commit with i-t-a entries" Nguyễn Thái Ngọc Duy
2012-02-06 10:57 ` [PATCH 1/4] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy
@ 2012-02-06 10:57 ` Nguyễn Thái Ngọc Duy
2012-02-06 19:50 ` Junio C Hamano
2012-02-06 21:10 ` Junio C Hamano
2012-02-06 10:57 ` [PATCH 3/4] commit: turn commit.ignoreIntentToAdd to true by default Nguyễn Thái Ngọc Duy
2012-02-06 10:57 ` [PATCH 4/4] commit: remove commit.ignoreIntentToAdd, assume it's always true Nguyễn Thái Ngọc Duy
3 siblings, 2 replies; 20+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
This is the start of deprecating current commit behavior (do not allow
commit as-is when there are i-t-a entries in index). Users will be
annoyed by a warning saying the behavior will change in 1.8.0 and asked
to set commit.ignoreIntentToAdd properly.
The plan after that is:
- Keep it this way until 1.8.0
- In 1.8.0, we consider the lack of commit.ignoreIntentToAdd means
"true". A deprecation date is set. The warning message is updated
stating that from that day, this config key will be ignored. The
behavior will be as if this config key is always "true".
- In the chosen release, remove support for this config key.
WRITE_TREE_IGNORE_INTENT_TO_ADD is set unconditionally.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 10 ++++++++++
Documentation/git-add.txt | 12 ++++++++++--
builtin/commit.c | 28 +++++++++++++++++++++++++---
cache-tree.c | 8 +++++---
cache-tree.h | 1 +
t/t2203-add-intent.sh | 21 ++++++++++++++++++++-
6 files changed, 71 insertions(+), 9 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..6ec81a8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -831,6 +831,16 @@ commit.template::
"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
specified user's home directory.
+commit.ignoreIntentToAdd::
+ Allow to commit the index as-is even if there are
+ intent-to-add entries (see option `-N` in linkgit:git-add[1])
+ in index. Set to `false` to disallow commit in this acase, or `true`
+ to allow it.
++
+By default, `git commit` refuses to commit as-is when you have intent-to-add
+entries. This will change in 1.8.0, where `git commit` allows it. If you
+prefer current behavior, please set it to `false`.
+
credential.helper::
Specify an external helper to be called when a username or
password credential is needed; the helper may consult external
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9c1d395..ec548ea 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -123,8 +123,16 @@ subdirectories.
Record only the fact that the path will be added later. An entry
for the path is placed in the index with no content. This is
useful for, among other things, showing the unstaged content of
- such files with `git diff` and committing them with `git commit
- -a`.
+ such files with `git diff`.
++
+Paths added with this option have intent-to-add flag in index. The
+flag is removed once real content is added or updated. By default you
+cannot commit the index as-is from until this flag is removed from all
+entries (i.e. all entries have real content). See commit.ignoreIntentToAdd
+regardless the flag.
++
+Committing with `git commit -a` or with selected paths works
+regardless the config key and the flag.
--refresh::
Don't add the file(s), but only refresh their stat()
diff --git a/builtin/commit.c b/builtin/commit.c
index bf42bb3..af3250c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -86,6 +86,7 @@ static int all, also, interactive, patch_interactive, only, amend, signoff;
static int edit_flag = -1; /* unspecified */
static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
static int no_post_rewrite, allow_empty_message;
+static int cache_tree_flags;
static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
static char *sign_commit;
@@ -117,6 +118,8 @@ static enum {
} status_format = STATUS_FORMAT_LONG;
static int status_show_branch;
+static int set_commit_ignoreintenttoadd;
+
static int opt_parse_m(const struct option *opt, const char *arg, int unset)
{
struct strbuf *buf = opt->value;
@@ -400,7 +403,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
fd = hold_locked_index(&index_lock, 1);
add_files_to_cache(also ? prefix : NULL, pathspec, 0);
refresh_cache_or_die(refresh_flags);
- update_main_cache_tree(WRITE_TREE_SILENT);
+ update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(&index_lock))
die(_("unable to write new_index file"));
@@ -420,8 +423,20 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
if (!pathspec || !*pathspec) {
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
+ if (!set_commit_ignoreintenttoadd) {
+ int i;
+ for (i = 0; i < active_nr; i++)
+ if (active_cache[i]->ce_flags & CE_INTENT_TO_ADD)
+ break;
+ if (i < active_nr)
+ warning(_("You are committing as-is with intent-to-add entries as the result of\n"
+ "\"git add -N\". Git currently forbids this case. This will change in\n"
+ "1.8.0 where intent-to-add entries are simply ignored when committing\n"
+ "as-is. Please look up document and set commit.ignoreIntentToAdd\n"
+ "properly to stop this warning."));
+ }
if (active_cache_changed) {
- update_main_cache_tree(WRITE_TREE_SILENT);
+ update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(&index_lock))
die(_("unable to write new_index file"));
@@ -870,7 +885,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
*/
discard_cache();
read_cache_from(index_file);
- if (update_main_cache_tree(0)) {
+ if (update_main_cache_tree(cache_tree_flags)) {
error(_("Error building trees"));
return 0;
}
@@ -1338,6 +1353,13 @@ static int git_commit_config(const char *k, const char *v, void *cb)
include_status = git_config_bool(k, v);
return 0;
}
+ if (!strcmp(k, "commit.ignoreintenttoadd")) {
+ set_commit_ignoreintenttoadd = 1;
+ if (git_config_bool(k, v))
+ cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
+ else
+ cache_tree_flags &= ~WRITE_TREE_IGNORE_INTENT_TO_ADD;
+ }
status = git_gpg_config(k, v, NULL);
if (status)
diff --git a/cache-tree.c b/cache-tree.c
index 16355d6..d0be159 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -159,7 +159,9 @@ static int verify_cache(struct cache_entry **cache,
funny = 0;
for (i = 0; i < entries; i++) {
struct cache_entry *ce = cache[i];
- if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) {
+ if (ce_stage(ce) ||
+ ((flags & WRITE_TREE_IGNORE_INTENT_TO_ADD) == 0 &&
+ (ce->ce_flags & CE_INTENT_TO_ADD))) {
if (silent)
return -1;
if (10 < ++funny) {
@@ -339,8 +341,8 @@ static int update_one(struct cache_tree *it,
mode, sha1_to_hex(sha1), entlen+baselen, path);
}
- if (ce->ce_flags & CE_REMOVE)
- continue; /* entry being removed */
+ if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
+ continue; /* entry being removed or placeholder */
strbuf_grow(&buffer, entlen + 100);
strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
diff --git a/cache-tree.h b/cache-tree.h
index d8cb2e9..af3b917 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -38,6 +38,7 @@ int update_main_cache_tree(int);
#define WRITE_TREE_IGNORE_CACHE_TREE 2
#define WRITE_TREE_DRY_RUN 4
#define WRITE_TREE_SILENT 8
+#define WRITE_TREE_IGNORE_INTENT_TO_ADD 16
/* error return codes */
#define WRITE_TREE_UNREADABLE_INDEX (-1)
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 58a3299..88a508e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -41,7 +41,26 @@ test_expect_success 'cannot commit with i-t-a entry' '
echo frotz >nitfol &&
git add rezrov &&
git add -N nitfol &&
- test_must_fail git commit
+ test_must_fail git commit -minitial
+'
+
+test_expect_success 'can commit tree with i-t-a entry' '
+ git reset --hard &&
+ echo xyzzy >rezrov &&
+ echo frotz >nitfol &&
+ git add rezrov &&
+ git add -N nitfol &&
+ git config commit.ignoreIntentToAdd true &&
+ git commit -m initial &&
+ git ls-tree -r HEAD >actual &&
+ cat >expected <<EOF &&
+100644 blob ce013625030ba8dba906f756967f9e9ca394464a elif
+100644 blob ce013625030ba8dba906f756967f9e9ca394464a file
+100644 blob cf7711b63209d0dbc2d030f7fe3513745a9880e4 rezrov
+EOF
+ test_cmp expected actual &&
+ git config commit.ignoreIntentToAdd false &&
+ git reset HEAD^
'
test_expect_success 'can commit with an unrelated i-t-a entry in index' '
--
1.7.8.36.g69ee2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] commit: turn commit.ignoreIntentToAdd to true by default
2012-02-06 10:57 [PATCH 0/4] Deprecate "not allow as-is commit with i-t-a entries" Nguyễn Thái Ngọc Duy
2012-02-06 10:57 ` [PATCH 1/4] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy
2012-02-06 10:57 ` [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries Nguyễn Thái Ngọc Duy
@ 2012-02-06 10:57 ` Nguyễn Thái Ngọc Duy
2012-02-06 20:03 ` Junio C Hamano
2012-02-06 10:57 ` [PATCH 4/4] commit: remove commit.ignoreIntentToAdd, assume it's always true Nguyễn Thái Ngọc Duy
3 siblings, 1 reply; 20+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
From now on, those who has not set commit.ignoreIntentToAdd can commit
as-is even if there are intent-to-add entries. Users are advised/annoyed
to switch to new behavior.
Support for "commit.ignoreIntentToAdd = true" will be dropped in future.
The placeholder "FIXME" needs to be replaced when it's decided what
release will drop config support.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 5 ++---
builtin/commit.c | 13 ++++++++-----
t/t2203-add-intent.sh | 4 ++--
3 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6ec81a8..f9a05ac 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -837,9 +837,8 @@ commit.ignoreIntentToAdd::
in index. Set to `false` to disallow commit in this acase, or `true`
to allow it.
+
-By default, `git commit` refuses to commit as-is when you have intent-to-add
-entries. This will change in 1.8.0, where `git commit` allows it. If you
-prefer current behavior, please set it to `false`.
+By default, `git commit` allows to commit as-is when you have intent-to-add
+entries. Support for this configuration variable will be dropped in FIXME.
credential.helper::
Specify an external helper to be called when a username or
diff --git a/builtin/commit.c b/builtin/commit.c
index af3250c..eb0ca49 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -423,17 +423,17 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
if (!pathspec || !*pathspec) {
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
- if (!set_commit_ignoreintenttoadd) {
+ if (!(cache_tree_flags & WRITE_TREE_IGNORE_INTENT_TO_ADD)) {
int i;
for (i = 0; i < active_nr; i++)
if (active_cache[i]->ce_flags & CE_INTENT_TO_ADD)
break;
if (i < active_nr)
warning(_("You are committing as-is with intent-to-add entries as the result of\n"
- "\"git add -N\". Git currently forbids this case. This will change in\n"
- "1.8.0 where intent-to-add entries are simply ignored when committing\n"
- "as-is. Please look up document and set commit.ignoreIntentToAdd\n"
- "properly to stop this warning."));
+ "\"git add -N\". Git currently forbids this case. But this is deprecated\n"
+ "support for this behavior will be dropped in FIXME.\n"
+ "Please look up document and set commit.ignoreIntentToAdd to true\n"
+ "or remove it."));
}
if (active_cache_changed) {
update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
@@ -1423,6 +1423,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
git_config(git_commit_config, &s);
determine_whence(&s);
+ if (!set_commit_ignoreintenttoadd)
+ cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
+
if (get_sha1("HEAD", sha1))
current_head = NULL;
else {
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 88a508e..09b8bbf 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -41,11 +41,11 @@ test_expect_success 'cannot commit with i-t-a entry' '
echo frotz >nitfol &&
git add rezrov &&
git add -N nitfol &&
- test_must_fail git commit -minitial
+ git commit -minitial
'
test_expect_success 'can commit tree with i-t-a entry' '
- git reset --hard &&
+ git reset --hard HEAD^ &&
echo xyzzy >rezrov &&
echo frotz >nitfol &&
git add rezrov &&
--
1.7.8.36.g69ee2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/4] commit: remove commit.ignoreIntentToAdd, assume it's always true
2012-02-06 10:57 [PATCH 0/4] Deprecate "not allow as-is commit with i-t-a entries" Nguyễn Thái Ngọc Duy
` (2 preceding siblings ...)
2012-02-06 10:57 ` [PATCH 3/4] commit: turn commit.ignoreIntentToAdd to true by default Nguyễn Thái Ngọc Duy
@ 2012-02-06 10:57 ` Nguyễn Thái Ngọc Duy
2012-02-06 20:05 ` Junio C Hamano
3 siblings, 1 reply; 20+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 9 ---------
builtin/commit.c | 24 +-----------------------
t/t2203-add-intent.sh | 2 +-
3 files changed, 2 insertions(+), 33 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f9a05ac..abeb82b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -831,15 +831,6 @@ commit.template::
"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
specified user's home directory.
-commit.ignoreIntentToAdd::
- Allow to commit the index as-is even if there are
- intent-to-add entries (see option `-N` in linkgit:git-add[1])
- in index. Set to `false` to disallow commit in this acase, or `true`
- to allow it.
-+
-By default, `git commit` allows to commit as-is when you have intent-to-add
-entries. Support for this configuration variable will be dropped in FIXME.
-
credential.helper::
Specify an external helper to be called when a username or
password credential is needed; the helper may consult external
diff --git a/builtin/commit.c b/builtin/commit.c
index eb0ca49..491cae1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -118,8 +118,6 @@ static enum {
} status_format = STATUS_FORMAT_LONG;
static int status_show_branch;
-static int set_commit_ignoreintenttoadd;
-
static int opt_parse_m(const struct option *opt, const char *arg, int unset)
{
struct strbuf *buf = opt->value;
@@ -423,18 +421,6 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
if (!pathspec || !*pathspec) {
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
- if (!(cache_tree_flags & WRITE_TREE_IGNORE_INTENT_TO_ADD)) {
- int i;
- for (i = 0; i < active_nr; i++)
- if (active_cache[i]->ce_flags & CE_INTENT_TO_ADD)
- break;
- if (i < active_nr)
- warning(_("You are committing as-is with intent-to-add entries as the result of\n"
- "\"git add -N\". Git currently forbids this case. But this is deprecated\n"
- "support for this behavior will be dropped in FIXME.\n"
- "Please look up document and set commit.ignoreIntentToAdd to true\n"
- "or remove it."));
- }
if (active_cache_changed) {
update_main_cache_tree(cache_tree_flags | WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
@@ -1353,13 +1339,6 @@ static int git_commit_config(const char *k, const char *v, void *cb)
include_status = git_config_bool(k, v);
return 0;
}
- if (!strcmp(k, "commit.ignoreintenttoadd")) {
- set_commit_ignoreintenttoadd = 1;
- if (git_config_bool(k, v))
- cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
- else
- cache_tree_flags &= ~WRITE_TREE_IGNORE_INTENT_TO_ADD;
- }
status = git_gpg_config(k, v, NULL);
if (status)
@@ -1423,8 +1402,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
git_config(git_commit_config, &s);
determine_whence(&s);
- if (!set_commit_ignoreintenttoadd)
- cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
+ cache_tree_flags |= WRITE_TREE_IGNORE_INTENT_TO_ADD;
if (get_sha1("HEAD", sha1))
current_head = NULL;
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 09b8bbf..7c7ab54 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -50,7 +50,7 @@ test_expect_success 'can commit tree with i-t-a entry' '
echo frotz >nitfol &&
git add rezrov &&
git add -N nitfol &&
- git config commit.ignoreIntentToAdd true &&
+ git config commit.ignoreIntentToAdd false &&
git commit -m initial &&
git ls-tree -r HEAD >actual &&
cat >expected <<EOF &&
--
1.7.8.36.g69ee2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-06 10:57 ` [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries Nguyễn Thái Ngọc Duy
@ 2012-02-06 19:50 ` Junio C Hamano
2012-02-07 0:43 ` Nguyen Thai Ngoc Duy
2012-02-06 21:10 ` Junio C Hamano
1 sibling, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2012-02-06 19:50 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> +commit.ignoreIntentToAdd::
> + Allow to commit the index as-is even if there are
> + intent-to-add entries (see option `-N` in linkgit:git-add[1])
> + in index. Set to `false` to disallow commit in this acase, or `true`
> + to allow it.
> ++
> +By default, `git commit` refuses to commit as-is when you have intent-to-add
> +entries. This will change in 1.8.0, where `git commit` allows it. If you
> +prefer current behavior, please set it to `false`.
s/acase/case/;
More importantly, if we allow it, what would be included and what would be
omitted from the final commit? What happens if we allow it is much more
important information than "we allow it even if there are".
When set to `true`, prevent `git commit` from creating a commit
from an index that has entries that were added with `git add -N`
but have not been updated with real contents, as the user may have
forgotten to tell the final contents for these entries. Setting
this to `false` makes `git commit` pretend as if these entries do
not exist in the index.
The default for this variable is `false`, but it will change to
`true` in later releases of git (perhaps 1.8.0). To ease the
transition, you may want to set it to `true` now and get used to
the new behaviour early, or you may want to set it to `false` to
keep the old behaviour a bit longer. We however expect to support
setting this to `false` (to keep the current behaviour) only for a
limited time after the default is changed to `true`.
Now, I removed the "intent to add" jargon from the above paragraph, the
description below can lose it as well.
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 9c1d395..ec548ea 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -123,8 +123,16 @@ subdirectories.
> Record only the fact that the path will be added later. An entry
> for the path is placed in the index with no content. This is
> useful for, among other things, showing the unstaged content of
> - such files with `git diff` and committing them with `git commit
> - -a`.
> + such files with `git diff`.
> ++
> +Paths added with this option have intent-to-add flag in index. The
> +flag is removed once real content is added or updated. By default you
> +cannot commit the index as-is from until this flag is removed from all
> +entries (i.e. all entries have real content). See commit.ignoreIntentToAdd
> +regardless the flag.
I cannot understand "regardlessthe flag" in the last sentence.
> ++
> +Committing with `git commit -a` or with selected paths works
> +regardless the config key and the flag.
Just an aside, not suggesting to change anything to the final text.
This is interesting in that the reason why "selected paths" works is a bit
subtle. You may have entries added with `-N` that are covered by the
pathspec, and those that are not covered by the pathspec. For the former
(i.e. those the user said "I'll decide the final contents later" but are
covered by pathspec), the user is telling the final contents to be in the
commit, so we know the final contents for them. For the latter (i.e.
those outside the pathspec), they are excluded and made to match the
version in the HEAD commit, so we know the final contents for them, too.
> + if (i < active_nr)
> + warning(_("You are committing as-is with intent-to-add entries as the result of\n"
> + "\"git add -N\". Git currently forbids this case.
Can we phrase this a bit better?
It is not like "forbids", but is "giving up because you didn't tell me
what content to include in the commit, even though you said you will tell
me later".
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] cache-tree: update API to take abitrary flags
2012-02-06 10:57 ` [PATCH 1/4] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy
@ 2012-02-06 19:51 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2012-02-06 19:51 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> int cache_tree_update(struct cache_tree *it,
> struct cache_entry **cache,
> int entries,
> - int missing_ok,
> - int dryrun,
> - int silent)
> + int flags)
Very nice ;-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] commit: turn commit.ignoreIntentToAdd to true by default
2012-02-06 10:57 ` [PATCH 3/4] commit: turn commit.ignoreIntentToAdd to true by default Nguyễn Thái Ngọc Duy
@ 2012-02-06 20:03 ` Junio C Hamano
2012-02-07 1:03 ` Nguyen Thai Ngoc Duy
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2012-02-06 20:03 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> - if (!set_commit_ignoreintenttoadd) {
Now no warning() is associated with testing of this flag, which means that
people who didn't get around to read the doc and react to the warnings in
the earlier releases will get _nothing_ when the real change hits them?
That sounds strangely bad. What am I missing?
> + if (!(cache_tree_flags & WRITE_TREE_IGNORE_INTENT_TO_ADD)) {
> int i;
> for (i = 0; i < active_nr; i++)
> if (active_cache[i]->ce_flags & CE_INTENT_TO_ADD)
> break;
> if (i < active_nr)
> warning(_("You are committing as-is with intent-to-add entries as the result of\n"
> - "\"git add -N\". Git currently forbids this case. This will change in\n"
> - "1.8.0 where intent-to-add entries are simply ignored when committing\n"
> - "as-is. Please look up document and set commit.ignoreIntentToAdd\n"
> - "properly to stop this warning."));
> + "\"git add -N\". Git currently forbids this case. But this is deprecated\n"
> + "support for this behavior will be dropped in FIXME.\n"
> + "Please look up document and set commit.ignoreIntentToAdd to true\n"
> + "or remove it."));
As this is marked with FIXME ;-)
At this point in the deprecation cycle, "currently forbids this case." is
not true at all. "You asked me to punt when you forgot to tell me the
final contents for i-t-a entries, so I am honoring your wish" is what is
happening here. Perhaps...
Git currently allows you to set commit.ignoreIntentToAdd to false
to remind you about the paths you may have forgotten to add the
real contents to before committing, but this support will be
removed in future versions of Git. Set commit.ignoreIntentToAdd
to `false` or remove the variable, and get used to the new
behaviour, to squelch this message.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] commit: remove commit.ignoreIntentToAdd, assume it's always true
2012-02-06 10:57 ` [PATCH 4/4] commit: remove commit.ignoreIntentToAdd, assume it's always true Nguyễn Thái Ngọc Duy
@ 2012-02-06 20:05 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2012-02-06 20:05 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 09b8bbf..7c7ab54 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -50,7 +50,7 @@ test_expect_success 'can commit tree with i-t-a entry' '
> echo frotz >nitfol &&
> git add rezrov &&
> git add -N nitfol &&
> - git config commit.ignoreIntentToAdd true &&
> + git config commit.ignoreIntentToAdd false &&
This deserves a comment to the effect that the variable is now *ignored*.
> git commit -m initial &&
> git ls-tree -r HEAD >actual &&
> cat >expected <<EOF &&
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-06 10:57 ` [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries Nguyễn Thái Ngọc Duy
2012-02-06 19:50 ` Junio C Hamano
@ 2012-02-06 21:10 ` Junio C Hamano
2012-02-06 21:13 ` Jonathan Nieder
` (2 more replies)
1 sibling, 3 replies; 20+ messages in thread
From: Junio C Hamano @ 2012-02-06 21:10 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 9c1d395..ec548ea 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -123,8 +123,16 @@ subdirectories.
> Record only the fact that the path will be added later. An entry
> for the path is placed in the index with no content. This is
> useful for, among other things, showing the unstaged content of
> - such files with `git diff` and committing them with `git commit
> - -a`.
> + such files with `git diff`.
> ++
> +Paths added with this option have intent-to-add flag in index. The
> +flag is removed once real content is added or updated. By default you
> +cannot commit the index as-is from until this flag is removed from all
> +entries (i.e. all entries have real content). See commit.ignoreIntentToAdd
> +regardless the flag.
> ++
> +Committing with `git commit -a` or with selected paths works
> +regardless the config key and the flag.
It still is not clear to me how best to sell this change to the end-user
community.
IIRC, the original motivation of intent-to-add "add -N" was in response to
users who curse Git because they often forget to add new files before
committing, and they wanted to say "Here I have a new file, it is not
complete yet, but I do not want it left out of the commit. While my memory
is fresh, let me tell Git to keep an eye on it, so that it can remind me
if I forget to add the final contents." For them, the current "did you
forget to add them? If so tell me the final contents for at least the
paths you will be changing with this commit" error was a perfect safety
solution.
It turned out that the benefits described we see above in the context,
"This is useful, among other things, ...", were of more value, and for
these use cases, i-t-a entries ceased to mean "I may forget, so I am
telling you now, please remind me when I say commit." And "did you
forget?" error is hinderance for them.
But does that mean nobody will ever need "please remind me lest I forget"?
Just the original requestor of the "add -N" feature may still be using
git, but more importantly, isn't it the case that those who have been
using it merely for the other side effect (e.g. 'git diff') sometimes want
the "please remind me" safety?
I suspect that some among 1 million Git users would want the "please
remind me", so a solution with configuration variable without command line
override is not ideal (command line without any configurability is fine as
long as we have a good default).
I am beginning to think "safety by default, which can be turned off by
learned users, but still can be turned on on demand" may be a lot easier
to sell this. That is:
- commit.ignoreIntentToAdd defaults to `false`; the default will never
change. The users can set it to `true`.
- "commit --ignore-intent-to-add" can be used without setting the
configuration or to defeat an explicit `false`, for a one-shot request.
- "commit --honor-intent-to-add" can be used to defeat an explicit
`true`, for a one-shot request.
The third one is a bit funny, as it is a way to bring back safety when the
user earlier decided that he does not need that kind of safety (i.e. "I
only say 'add -N' for `diff` and stuff, I will never forget to add real
contents before committing"), so it will almost never be used, because
these users who set 'ignoreIntentToAdd = true' do _not_ expect Git to help
them in remembering to add the real contents. And having to add a funny
option just for the sake of completeness is often an indication that there
is something fundamentally wrong in the system that the option tries to
express an interface into it.
Without conclusion... Sigh...
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-06 21:10 ` Junio C Hamano
@ 2012-02-06 21:13 ` Jonathan Nieder
2012-02-06 21:48 ` Junio C Hamano
2012-02-07 0:58 ` Nguyen Thai Ngoc Duy
2012-02-07 6:10 ` Junio C Hamano
2 siblings, 1 reply; 20+ messages in thread
From: Jonathan Nieder @ 2012-02-06 21:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
Hi,
Junio C Hamano wrote:
> IIRC, the original motivation of intent-to-add "add -N" was in response to
> users who curse Git because they often forget to add new files before
> committing, and they wanted to say "Here I have a new file, it is not
> complete yet, but I do not want it left out of the commit. While my memory
> is fresh, let me tell Git to keep an eye on it, so that it can remind me
> if I forget to add the final contents."
I agree with everything up to here. But I believe these people were
_already_ paying attention to "git status" output from the commandline
and in the editor window when they run "git commit", to notice other
changes they forgot to add, too. I don't think this series would
inconvenience them.
Hoping I can find time to look over the other changes soon.
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-06 21:13 ` Jonathan Nieder
@ 2012-02-06 21:48 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2012-02-06 21:48 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Nguyễn Thái Ngọc Duy, git
Jonathan Nieder <jrnieder@gmail.com> writes:
> Junio C Hamano wrote:
>
>> IIRC, the original motivation of intent-to-add "add -N" was in response to
>> users who curse Git because they often forget to add new files before
>> committing, and they wanted to say "Here I have a new file, it is not
>> complete yet, but I do not want it left out of the commit. While my memory
>> is fresh, let me tell Git to keep an eye on it, so that it can remind me
>> if I forget to add the final contents."
>
> I agree with everything up to here. But I believe these people were
> _already_ paying attention to "git status" output from the commandline
> and in the editor window when they run "git commit", to notice other
> changes they forgot to add, too. I don't think this series would
> inconvenience them.
That means that you are willing to declare that nobody will ever need
"please remind me lest I forget". Not just the original requestor of the
"add -N" feature, but absolutely nobody else.
Then the deprecation sequence presented in this series is fine. The
wording to sell that "removal of misfeature" to the end user community
needs to be well thought out, though.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-06 19:50 ` Junio C Hamano
@ 2012-02-07 0:43 ` Nguyen Thai Ngoc Duy
2012-02-07 0:59 ` Jonathan Nieder
0 siblings, 1 reply; 20+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2012-02-07 0:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
2012/2/7 Junio C Hamano <gitster@pobox.com>:
>> + if (i < active_nr)
>> + warning(_("You are committing as-is with intent-to-add entries as the result of\n"
>> + "\"git add -N\". Git currently forbids this case.
>
> Can we phrase this a bit better?
>
> It is not like "forbids", but is "giving up because you didn't tell me
> what content to include in the commit, even though you said you will tell
> me later".
"rejects"? I would rather say "see `git add -N` man page for more
explanation" than putting it here. The warning is quite long as it is
right now.
--
Duy
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-06 21:10 ` Junio C Hamano
2012-02-06 21:13 ` Jonathan Nieder
@ 2012-02-07 0:58 ` Nguyen Thai Ngoc Duy
2012-02-07 1:13 ` Junio C Hamano
2012-02-07 6:10 ` Junio C Hamano
2 siblings, 1 reply; 20+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2012-02-07 0:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
2012/2/7 Junio C Hamano <gitster@pobox.com>:
> It still is not clear to me how best to sell this change to the end-user
> community.
>
> IIRC, the original motivation of intent-to-add "add -N" was in response to
> users who curse Git because they often forget to add new files before
> committing, and they wanted to say "Here I have a new file, it is not
> complete yet, but I do not want it left out of the commit. While my memory
> is fresh, let me tell Git to keep an eye on it, so that it can remind me
> if I forget to add the final contents." For them, the current "did you
> forget to add them? If so tell me the final contents for at least the
> paths you will be changing with this commit" error was a perfect safety
> solution.
>
> It turned out that the benefits described we see above in the context,
> "This is useful, among other things, ...", were of more value, and for
> these use cases, i-t-a entries ceased to mean "I may forget, so I am
> telling you now, please remind me when I say commit." And "did you
> forget?" error is hinderance for them.
>
> But does that mean nobody will ever need "please remind me lest I forget"?
> Just the original requestor of the "add -N" feature may still be using
> git, but more importantly, isn't it the case that those who have been
> using it merely for the other side effect (e.g. 'git diff') sometimes want
> the "please remind me" safety?
>
> I suspect that some among 1 million Git users would want the "please
> remind me", so a solution with configuration variable without command line
> override is not ideal (command line without any configurability is fine as
> long as we have a good default).
Which is why I prefer adding a new configuration variable (and
optionally a command line option) instead of deprecating current
behavior, because (being lazy) I never be able to find "some among 1
million" so I'm fine with assuming there are some among 1 million that
favors safety over convenience.
> I am beginning to think "safety by default, which can be turned off by
> learned users, but still can be turned on on demand" may be a lot easier
> to sell this. That is:
>
> - commit.ignoreIntentToAdd defaults to `false`; the default will never
> change. The users can set it to `true`.
>
> - "commit --ignore-intent-to-add" can be used without setting the
> configuration or to defeat an explicit `false`, for a one-shot request.
>
> - "commit --honor-intent-to-add" can be used to defeat an explicit
> `true`, for a one-shot request.
>
> The third one is a bit funny, as it is a way to bring back safety when the
> user earlier decided that he does not need that kind of safety (i.e. "I
> only say 'add -N' for `diff` and stuff, I will never forget to add real
> contents before committing"), so it will almost never be used, because
> these users who set 'ignoreIntentToAdd = true' do _not_ expect Git to help
> them in remembering to add the real contents. And having to add a funny
> option just for the sake of completeness is often an indication that there
> is something fundamentally wrong in the system that the option tries to
> express an interface into it.
Well, that --honor-intent-to-add could be renamed as
--no-ignore-intent-to-add. The --[no-]ignore-intent-to-add pair
functions as a way to override default behavior/config var. No extra
could required. "git commit -h" just does not show it.
We need better option/config names though, --ignore-intent-to-add
looks way too long to type and it's not clear what it does without
looking up "git add -N".
--
Duy
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-07 0:43 ` Nguyen Thai Ngoc Duy
@ 2012-02-07 0:59 ` Jonathan Nieder
0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Nieder @ 2012-02-07 0:59 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git
Nguyen Thai Ngoc Duy wrote:
> 2012/2/7 Junio C Hamano <gitster@pobox.com>:
>>> + if (i < active_nr)
>>> + warning(_("You are committing as-is with intent-to-add entries as the result of\n"
>>> + "\"git add -N\". Git currently forbids this case.
>>
>> Can we phrase this a bit better?
>>
>> It is not like "forbids", but is "giving up because you didn't tell me
>> what content to include in the commit, even though you said you will tell
>> me later".
>
> "rejects"? I would rather say "see `git add -N` man page for more
> explanation" than putting it here. The warning is quite long as it is
> right now.
If I ruled the world, it would say something like this:
error: you intended to add "foo.c" but did not add it; not committing
hint: to commit all changes to tracked files, use "commit -a"
hint: to commit without adding "foo.c", use "commit --ignore-intent-to-add", which may become the default in future versions of git
But without the long line. ;-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] commit: turn commit.ignoreIntentToAdd to true by default
2012-02-06 20:03 ` Junio C Hamano
@ 2012-02-07 1:03 ` Nguyen Thai Ngoc Duy
0 siblings, 0 replies; 20+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2012-02-07 1:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
2012/2/7 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> - if (!set_commit_ignoreintenttoadd) {
>
> Now no warning() is associated with testing of this flag, which means that
> people who didn't get around to read the doc and react to the warnings in
> the earlier releases will get _nothing_ when the real change hits them?
>
> That sounds strangely bad. What am I missing?
Well if they stumble upon this case during the previous deprecation
phase, they ought to set commit.ignoreIntentToAdd. If it's not set,
they probably do not have prior experience with this feature. With
their fresh mind, they hopefully learn the new default behavior via
to-be-updated git-add.txt so no warnings for them.
--
Duy
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-07 0:58 ` Nguyen Thai Ngoc Duy
@ 2012-02-07 1:13 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2012-02-07 1:13 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jonathan Nieder
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 2012/2/7 Junio C Hamano <gitster@pobox.com>:
>
>> I suspect that some among 1 million Git users would want the "please
>> remind me", so a solution with configuration variable without command line
>> override is not ideal (command line without any configurability is fine as
>> long as we have a good default).
>
> Which is why I prefer adding a new configuration variable (and
> optionally a command line option) instead of deprecating current
> behavior, because (being lazy) I never be able to find "some among 1
> million" so I'm fine with assuming there are some among 1 million that
> favors safety over convenience.
If there is one thing I want to absolutely avoid, it is to split the
userbase into many pieces by giving sticky configuration variables, so
the above argument is not a good starting point.
>> The third one is a bit funny, as it is a way to bring back safety when the
>> .... And having to add a funny
>> option just for the sake of completeness is often an indication that there
>> is something fundamentally wrong in the system that the option tries to
>> express an interface into it.
>
> Well, that --honor-intent-to-add could be renamed as
> --no-ignore-intent-to-add.
I wasn't talking about the name at all. What is _funny_ is the semantics.
A "by default unsafe" configuration introduces a need for an option to be
extra careful only when matters, but "an option" to be extra careful by
definition is easy to forget, so it is no longer a safety at all, iow,
people who want "by default unsafe" will get "always unsafe". And it
probably is perfectly fine because to them, forgetting to add 'add -N'
entries is not a mistake at all, but always is a deliberate act.
Another thing I am somewhat worried about is if there are existing scripts
that create commits and relies on the current "we cannot commit because
the final contents is not known yet". I didn't check but for example how
well does "git stash" work when the default is flipped to "just ignore"?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-06 21:10 ` Junio C Hamano
2012-02-06 21:13 ` Jonathan Nieder
2012-02-07 0:58 ` Nguyen Thai Ngoc Duy
@ 2012-02-07 6:10 ` Junio C Hamano
2012-02-07 6:26 ` Nguyen Thai Ngoc Duy
2 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2012-02-07 6:10 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
Junio C Hamano <gitster@pobox.com> writes:
> It still is not clear to me how best to sell this change to the end-user
> community.
OK, sorry for wavering.
Unless you are doing "commit -a" or "commit pathspec", you are responsible
for adding all contents you want to have in the commit before you run the
"git commit" command (and for the purpose of this statement, "add -N" to
tell Git to keey an eye on a path does _not_ add contents). A change to
the file in the working tree that is left unadded is what you decided to
deliberately leave out of the commit, be it a change to a path already in
HEAD, or a path marked with "add -N". Forgetting to add modified file and
forgetting to add a file you earlier used "add -N" amount to the same kind
of risk, and "git status" is the way to make sure your partial commit has
exactly what you want (if you are not worried about partial commit, you
would be doing "commit -a", so the "safety" is a moot point).
I was worried too much about backward compatibility and was blind to the
above, and was mistakenly defending a false "safety" that did not exist at
all. Sorry for wasting everybody's time.
So let's bite the bullet and admit in the Release Notes that the current
behaviour was a UI mistake based on the misguided assumption that we can
give some kind of "safety" by committing when there are "add -N" entries
in the index, which is untrue, and we are fixing it in the new release.
We do not need configuration nor command line options.
Let me try to reroll your patch tomorrow (unless you beat me to it) and
see if I can come up with an easy-to-understand explanation to it.
Thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-07 6:10 ` Junio C Hamano
@ 2012-02-07 6:26 ` Nguyen Thai Ngoc Duy
2012-02-07 7:57 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2012-02-07 6:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
2012/2/7 Junio C Hamano <gitster@pobox.com>:
> Unless you are doing "commit -a" or "commit pathspec", you are responsible
> for adding all contents you want to have in the commit before you run the
> "git commit" command (and for the purpose of this statement, "add -N" to
> tell Git to keey an eye on a path does _not_ add contents). A change to
> the file in the working tree that is left unadded is what you decided to
> deliberately leave out of the commit, be it a change to a path already in
> HEAD, or a path marked with "add -N". Forgetting to add modified file and
> forgetting to add a file you earlier used "add -N" amount to the same kind
> of risk, and "git status" is the way to make sure your partial commit has
> exactly what you want (if you are not worried about partial commit, you
> would be doing "commit -a", so the "safety" is a moot point).
We need something in the commit message so that 5 years from now, when
someone raises the issue again, (s)he does not have to search the mail
archive. May I steal the above paragraph, maybe rephrase a little bit,
for commit message?
--
Duy
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
2012-02-07 6:26 ` Nguyen Thai Ngoc Duy
@ 2012-02-07 7:57 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2012-02-07 7:57 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jonathan Nieder
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 2012/2/7 Junio C Hamano <gitster@pobox.com>:
>> Unless you are doing "commit -a" or "commit pathspec", you are responsible
>> for adding all contents you want to have in the commit before you run the
>> "git commit" command (and for the purpose of this statement, "add -N" to
>> tell Git to keey an eye on a path does _not_ add contents). A change to
>> the file in the working tree that is left unadded is what you decided to
>> deliberately leave out of the commit, be it a change to a path already in
>> HEAD, or a path marked with "add -N". Forgetting to add modified file and
>> forgetting to add a file you earlier used "add -N" amount to the same kind
>> of risk, and "git status" is the way to make sure your partial commit has
>> exactly what you want (if you are not worried about partial commit, you
>> would be doing "commit -a", so the "safety" is a moot point).
>
> We need something in the commit message so that 5 years from now, when
> someone raises the issue again, (s)he does not have to search the mail
> archive. May I steal the above paragraph, maybe rephrase a little bit,
> for commit message?
Surely, and thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2012-02-07 7:57 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-06 10:57 [PATCH 0/4] Deprecate "not allow as-is commit with i-t-a entries" Nguyễn Thái Ngọc Duy
2012-02-06 10:57 ` [PATCH 1/4] cache-tree: update API to take abitrary flags Nguyễn Thái Ngọc Duy
2012-02-06 19:51 ` Junio C Hamano
2012-02-06 10:57 ` [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries Nguyễn Thái Ngọc Duy
2012-02-06 19:50 ` Junio C Hamano
2012-02-07 0:43 ` Nguyen Thai Ngoc Duy
2012-02-07 0:59 ` Jonathan Nieder
2012-02-06 21:10 ` Junio C Hamano
2012-02-06 21:13 ` Jonathan Nieder
2012-02-06 21:48 ` Junio C Hamano
2012-02-07 0:58 ` Nguyen Thai Ngoc Duy
2012-02-07 1:13 ` Junio C Hamano
2012-02-07 6:10 ` Junio C Hamano
2012-02-07 6:26 ` Nguyen Thai Ngoc Duy
2012-02-07 7:57 ` Junio C Hamano
2012-02-06 10:57 ` [PATCH 3/4] commit: turn commit.ignoreIntentToAdd to true by default Nguyễn Thái Ngọc Duy
2012-02-06 20:03 ` Junio C Hamano
2012-02-07 1:03 ` Nguyen Thai Ngoc Duy
2012-02-06 10:57 ` [PATCH 4/4] commit: remove commit.ignoreIntentToAdd, assume it's always true Nguyễn Thái Ngọc Duy
2012-02-06 20:05 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).