Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode
Junio C Hamano writes:
> On Wed, Feb 5, 2014 at 3:48 PM, Duy Nguyen wrote:
>> No no. I found that duplicate, but I did not suggest removing it
>> because it is needed there..
>
> Hmph, if that is the case, we probably should make it the
> responsibility of the calling side to actually mark ce->flags with the
> bit (which would also mean the function must be renamed to make it
> clear that it does not mark).
After looking at the codepath that uses the record_intent_to_add()
before this patch, I am coming to the conclusion that it is the
right thing to do after all. The code appears in this section:
if (!intent_only) {
if (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT))
return error("unable to index file %s", path);
} else
record_intent_to_add(ce);
which tells (at least) me: "We are not adding the contents of this
path, so we do not run index_path(); instead we call this helper
function to set the object name in ce to represent an intent-to-add
entry".
So I'll rename it to set_object_name_for_intent_to_add_entry() or
something, restore that flag manipulation back to the caller, and
add another to the new caller, and requeue.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode
On Wed, Feb 5, 2014 at 3:48 PM, Duy Nguyen wrote: > No no. I found that duplicate, but I did not suggest removing it > because it is needed there.. Hmph, if that is the case, we probably should make it the responsibility of the calling side to actually mark ce->flags with the bit (which would also mean the function must be renamed to make it clear that it does not mark). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode
On Thu, Feb 6, 2014 at 1:25 AM, Junio C Hamano wrote: >> Yes, indeed. I wonder why your new test did not notice it, though >> ;-) > > ... and the answer turns out to be that it was not testing the right > thing. On top of that faulty version, this will fix it. Yes, write-tree should test that well. > Your suggestion to move CE_INTENT_TO_ADD to mark-intent-to-add makes > sense but a caller needs to be adjusted to drop the duplicated flag > manipulation. No no. I found that duplicate, but I did not suggest removing it because it is needed there.. > @@ -613,8 +614,6 @@ int add_to_index(struct index_state *istate, const char > *path, struct stat *st, > ce->ce_namelen = namelen; > if (!intent_only) > fill_stat_cache_info(ce, st); > - else > - ce->ce_flags |= CE_INTENT_TO_ADD; > > if (trust_executable_bit && has_symlinks) > ce->ce_mode = create_ce_mode(st_mode); A few lines down, there's ie_match_stat() call that will check CE_INTENT_TO_ADD and returns "changed" immediately without looking at stat data. If stat info is used, it may (not so sure) return "not changed", the exit path is taken and mark_intent_to_add() is ignored. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode
Junio C Hamano writes:
> Duy Nguyen writes:
>
>> On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote:
>>> Junio C Hamano writes:
>>>
>>> > While I do not have any problem with adding an optional "keep lost
>>> > paths as intent-to-add entries" feature, I am not sure why this has
>>> > to be so different from the usual add-cache-entry codepath. The
>>> > if/elseif chain you are touching inside this loop does:
>>> >
>>> > - If the tree you are resetting to has something at the path
>>> >(which is different from the current index, obviously), create
>>> >a cache entry to represent that state from the tree and stuff
>>> >it in the index;
>>> >
>>> > - Otherwise, the tree you are resetting to does not have that
>>> >path. We used to say "remove it from the index", but now we have
>>> >an option to instead add it as an intent-to-add entry.
>>> >
>>> > So, why doesn't the new codepath do exactly the same thing as the
>>> > first branch of the if/else chain and call add_cache_entry but with
>>> > a ce marked with CE_INTENT_TO_ADD? That would parallel what happens
>>> > in "git add -N" better, I would think, no?
>>>
>>> In other words, something along this line, perhaps?
>>
>>
>>
>> Yes. But you need something like this on top to actually set
>> CE_INTENT_TO_ADD
>
> Yes, indeed. I wonder why your new test did not notice it, though
> ;-)
... and the answer turns out to be that it was not testing the right
thing. On top of that faulty version, this will fix it.
Your suggestion to move CE_INTENT_TO_ADD to mark-intent-to-add makes
sense but a caller needs to be adjusted to drop the duplicated flag
manipulation.
read-cache.c | 3 +--
t/t7102-reset.sh | 6 ++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 325d193..5b8102a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -584,6 +584,7 @@ void mark_intent_to_add(struct cache_entry *ce)
unsigned char sha1[20];
if (write_sha1_file("", 0, blob_type, sha1))
die("cannot create an empty blob in the object database");
+ ce->ce_flags |= CE_INTENT_TO_ADD;
hashcpy(ce->sha1, sha1);
}
@@ -613,8 +614,6 @@ int add_to_index(struct index_state *istate, const char
*path, struct stat *st,
ce->ce_namelen = namelen;
if (!intent_only)
fill_stat_cache_info(ce, st);
- else
- ce->ce_flags |= CE_INTENT_TO_ADD;
if (trust_executable_bit && has_symlinks)
ce->ce_mode = create_ce_mode(st_mode);
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 642920a..bc0846f 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -539,6 +539,12 @@ test_expect_success 'reset -N keeps removed files as
intent-to-add' '
echo new-file >new-file &&
git add new-file &&
git reset -N HEAD &&
+
+ tree=$(git write-tree) &&
+ git ls-tree $tree new-file >actual &&
+ >expect &&
+ test_cmp expect actual &&
+
git diff --name-only >actual &&
echo new-file >expect &&
test_cmp expect actual
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode
Duy Nguyen writes:
> On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote:
>> Junio C Hamano writes:
>>
>> > While I do not have any problem with adding an optional "keep lost
>> > paths as intent-to-add entries" feature, I am not sure why this has
>> > to be so different from the usual add-cache-entry codepath. The
>> > if/elseif chain you are touching inside this loop does:
>> >
>> > - If the tree you are resetting to has something at the path
>> >(which is different from the current index, obviously), create
>> >a cache entry to represent that state from the tree and stuff
>> >it in the index;
>> >
>> > - Otherwise, the tree you are resetting to does not have that
>> >path. We used to say "remove it from the index", but now we have
>> >an option to instead add it as an intent-to-add entry.
>> >
>> > So, why doesn't the new codepath do exactly the same thing as the
>> > first branch of the if/else chain and call add_cache_entry but with
>> > a ce marked with CE_INTENT_TO_ADD? That would parallel what happens
>> > in "git add -N" better, I would think, no?
>>
>> In other words, something along this line, perhaps?
>
>
>
> Yes. But you need something like this on top to actually set
> CE_INTENT_TO_ADD
Yes, indeed. I wonder why your new test did not notice it, though
;-)
>
> -- 8< --
> diff --git a/read-cache.c b/read-cache.c
> index 325d193..87f1367 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -585,6 +585,7 @@ void mark_intent_to_add(struct cache_entry *ce)
> if (write_sha1_file("", 0, blob_type, sha1))
> die("cannot create an empty blob in the object database");
> hashcpy(ce->sha1, sha1);
> + ce->ce_flags |= CE_INTENT_TO_ADD;
> }
>
> int add_to_index(struct index_state *istate, const char *path, struct stat
> *st, int flags)
> -- 8< --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode
On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote:
> Junio C Hamano writes:
>
> > While I do not have any problem with adding an optional "keep lost
> > paths as intent-to-add entries" feature, I am not sure why this has
> > to be so different from the usual add-cache-entry codepath. The
> > if/elseif chain you are touching inside this loop does:
> >
> > - If the tree you are resetting to has something at the path
> >(which is different from the current index, obviously), create
> >a cache entry to represent that state from the tree and stuff
> >it in the index;
> >
> > - Otherwise, the tree you are resetting to does not have that
> >path. We used to say "remove it from the index", but now we have
> >an option to instead add it as an intent-to-add entry.
> >
> > So, why doesn't the new codepath do exactly the same thing as the
> > first branch of the if/else chain and call add_cache_entry but with
> > a ce marked with CE_INTENT_TO_ADD? That would parallel what happens
> > in "git add -N" better, I would think, no?
>
> In other words, something along this line, perhaps?
Yes. But you need something like this on top to actually set
CE_INTENT_TO_ADD
-- 8< --
diff --git a/read-cache.c b/read-cache.c
index 325d193..87f1367 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -585,6 +585,7 @@ void mark_intent_to_add(struct cache_entry *ce)
if (write_sha1_file("", 0, blob_type, sha1))
die("cannot create an empty blob in the object database");
hashcpy(ce->sha1, sha1);
+ ce->ce_flags |= CE_INTENT_TO_ADD;
}
int add_to_index(struct index_state *istate, const char *path, struct stat
*st, int flags)
-- 8< --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode
Junio C Hamano writes:
> While I do not have any problem with adding an optional "keep lost
> paths as intent-to-add entries" feature, I am not sure why this has
> to be so different from the usual add-cache-entry codepath. The
> if/elseif chain you are touching inside this loop does:
>
> - If the tree you are resetting to has something at the path
>(which is different from the current index, obviously), create
>a cache entry to represent that state from the tree and stuff
>it in the index;
>
> - Otherwise, the tree you are resetting to does not have that
>path. We used to say "remove it from the index", but now we have
>an option to instead add it as an intent-to-add entry.
>
> So, why doesn't the new codepath do exactly the same thing as the
> first branch of the if/else chain and call add_cache_entry but with
> a ce marked with CE_INTENT_TO_ADD? That would parallel what happens
> in "git add -N" better, I would think, no?
In other words, something along this line, perhaps?
-- >8 --
From: Nguyễn Thái Ngọc Duy
Date: Tue, 4 Feb 2014 09:20:09 +0700
When --mixed is used, entries could be removed from index if the
target ref does not have them. When "reset" is used in preparation for
commit spliting (in a dirty worktree), it could be hard to track what
files to be added back. The new option --intent-to-add simplifies it
by marking all removed files intent-to-add.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-reset.txt | 5 -
builtin/reset.c | 38 ++
cache.h | 1 +
read-cache.c| 4 ++--
t/t7102-reset.sh| 9 +
5 files changed, 42 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index f445cb3..a077ba0 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,7 +10,7 @@ SYNOPSIS
[verse]
'git reset' [-q] [] [--] ...
'git reset' (--patch | -p) [] [--] [...]
-'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] []
+'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] []
DESCRIPTION
---
@@ -60,6 +60,9 @@ section of linkgit:git-add[1] to learn how to operate the
`--patch` mode.
Resets the index but not the working tree (i.e., the changed files
are preserved but not marked for commit) and reports what has not
been updated. This is the default action.
++
+If `-N` is specified, removed paths are marked as intent-to-add (see
+linkgit:git-add[1]).
--hard::
Resets the index and working tree. Any changes to tracked files in the
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..d363bc5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -116,25 +116,32 @@ static void update_index_from_diff(struct
diff_queue_struct *q,
struct diff_options *opt, void *data)
{
int i;
+ int intent_to_add = *(int *)data;
for (i = 0; i < q->nr; i++) {
struct diff_filespec *one = q->queue[i]->one;
- if (one->mode && !is_null_sha1(one->sha1)) {
- struct cache_entry *ce;
- ce = make_cache_entry(one->mode, one->sha1, one->path,
- 0, 0);
- if (!ce)
- die(_("make_cache_entry failed for path '%s'"),
- one->path);
- add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
- ADD_CACHE_OK_TO_REPLACE);
- } else
+ int is_missing = !(one->mode && !is_null_sha1(one->sha1));
+ struct cache_entry *ce;
+
+ if (is_missing && !intent_to_add) {
remove_file_from_cache(one->path);
+ continue;
+ }
+
+ ce = make_cache_entry(one->mode, one->sha1, one->path,
+ 0, 0);
+ if (!ce)
+ die(_("make_cache_entry failed for path '%s'"),
+ one->path);
+ if (is_missing)
+ mark_intent_to_add(ce);
+ add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
ADD_CACHE_OK_TO_REPLACE);
}
}
static int read_from_tree(const struct pathspec *pathspec,
- unsigned char *tree_sha1)
+ unsigned char *tree_sha1,
+ int intent_to_add)
{
struct diff_options opt;
@@ -142,6 +149,7 @@ static int read_from_tree(const struct pathspec *pathspec,
copy_pathspec(&opt.pathspec, pathspec);
opt.output_format = DIFF_FORMAT_CALLBACK;
opt.format_callback = update_index_from_diff;
+ opt.format_callback_data = &intent_to_add;
if (do_diff_cache(tree_sha1, &opt))
return 1;
@@ -258,6 +266,7 @@ int cmd_reset(int argc, const char **ar
Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode
Nguyễn Thái Ngọc Duy writes:
> @@ -128,13 +129,20 @@ static void update_index_from_diff(struct
> diff_queue_struct *q,
> one->path);
> add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
> ADD_CACHE_OK_TO_REPLACE);
> + } else if (*intent_to_add) {
> + int pos = cache_name_pos(one->path, strlen(one->path));
> + if (pos < 0)
> + die(_("%s does not exist in index"),
> + one->path);
> + set_intent_to_add(&the_index, active_cache[pos]);
While I do not have any problem with adding an optional "keep lost
paths as intent-to-add entries" feature, I am not sure why this has
to be so different from the usual add-cache-entry codepath. The
if/elseif chain you are touching inside this loop does:
- If the tree you are resetting to has something at the path
(which is different from the current index, obviously), create
a cache entry to represent that state from the tree and stuff
it in the index;
- Otherwise, the tree you are resetting to does not have that
path. We used to say "remove it from the index", but now we have
an option to instead add it as an intent-to-add entry.
So, why doesn't the new codepath do exactly the same thing as the
first branch of the if/else chain and call add_cache_entry but with
a ce marked with CE_INTENT_TO_ADD? That would parallel what happens
in "git add -N" better, I would think, no?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] reset: support "--mixed --intent-to-add" mode
When --mixed is used, entries could be removed from index if the
target ref does not have them. When "reset" is used in preparation for
commit spliting (in a dirty worktree), it could be hard to track what
files to be added back. The new option --intent-to-add simplifies it
by marking all removed files intent-to-add.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-reset.txt | 5 -
builtin/reset.c | 19 +--
cache.h | 1 +
read-cache.c| 9 +
t/t7102-reset.sh| 9 +
5 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index f445cb3..a077ba0 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,7 +10,7 @@ SYNOPSIS
[verse]
'git reset' [-q] [] [--] ...
'git reset' (--patch | -p) [] [--] [...]
-'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] []
+'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] []
DESCRIPTION
---
@@ -60,6 +60,9 @@ section of linkgit:git-add[1] to learn how to operate the
`--patch` mode.
Resets the index but not the working tree (i.e., the changed files
are preserved but not marked for commit) and reports what has not
been updated. This is the default action.
++
+If `-N` is specified, removed paths are marked as intent-to-add (see
+linkgit:git-add[1]).
--hard::
Resets the index and working tree. Any changes to tracked files in the
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..f34eab4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -116,6 +116,7 @@ static void update_index_from_diff(struct diff_queue_struct
*q,
struct diff_options *opt, void *data)
{
int i;
+ int *intent_to_add = data;
for (i = 0; i < q->nr; i++) {
struct diff_filespec *one = q->queue[i]->one;
@@ -128,13 +129,20 @@ static void update_index_from_diff(struct
diff_queue_struct *q,
one->path);
add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
ADD_CACHE_OK_TO_REPLACE);
+ } else if (*intent_to_add) {
+ int pos = cache_name_pos(one->path, strlen(one->path));
+ if (pos < 0)
+ die(_("%s does not exist in index"),
+ one->path);
+ set_intent_to_add(&the_index, active_cache[pos]);
} else
remove_file_from_cache(one->path);
}
}
static int read_from_tree(const struct pathspec *pathspec,
- unsigned char *tree_sha1)
+ unsigned char *tree_sha1,
+ int intent_to_add)
{
struct diff_options opt;
@@ -142,6 +150,7 @@ static int read_from_tree(const struct pathspec *pathspec,
copy_pathspec(&opt.pathspec, pathspec);
opt.output_format = DIFF_FORMAT_CALLBACK;
opt.format_callback = update_index_from_diff;
+ opt.format_callback_data = &intent_to_add;
if (do_diff_cache(tree_sha1, &opt))
return 1;
@@ -258,6 +267,7 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
const char *rev;
unsigned char sha1[20];
struct pathspec pathspec;
+ int intent_to_add = 0;
const struct option options[] = {
OPT__QUIET(&quiet, N_("be quiet, only report errors")),
OPT_SET_INT(0, "mixed", &reset_type,
@@ -270,6 +280,8 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
OPT_SET_INT(0, "keep", &reset_type,
N_("reset HEAD but keep local changes"), KEEP),
OPT_BOOL('p', "patch", &patch_mode, N_("select hunks
interactively")),
+ OPT_BOOL('N', "intent-to-add", &intent_to_add,
+ N_("record only the fact that removed paths
will be added later")),
OPT_END()
};
@@ -327,6 +339,9 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
die(_("%s reset is not allowed in a bare repository"),
_(reset_type_names[reset_type]));
+ if (intent_to_add && reset_type != MIXED)
+ die(_("-N can only be used with --mixed"));
+
/* Soft reset does not touch the index file nor the working tree
* at all, but requires them in a good order. Other resets reset
* the index file to the tree object we are switching to. */
@@ -338,7 +353,7 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
int newfd = hold_locked_index(lock, 1);
if (reset_type == MIXED) {
int flags = quiet ? REFRESH_QUIET :
REFRESH_IN_PORCELAIN;
- if (read_f
