Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
On Tue, Sep 27, 2016 at 5:58 PM, Duy Nguyen wrote: >> but then you also have to change the type of xdl_opts >> to uint64_t, which in turn means that you will have to change the >> definition of xpparam_t's "flags" field from unsigned long to uint64_t. > > I miss a connection here. This new flag is intended to be used in > "flags" field in struct diff_options. Is there any chance it can be > set on xdl_opts (of the same struct, I assume)? > >> Maybe this can be avoided? > > I don't see a good way to avoid it. We normally enable or disable diff > features as bit flags and now we run out of bits. Adding something > like "flags2" works, but not pretty. Any suggestion is welcome. Never mind. I think I found some way that does not look particularly bad. -- Duy
Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
(sorry for a very late reply, I'm just picking this series up again) On Thu, Jun 9, 2016 at 11:18 PM, Johannes Schindelin wrote: > Hi Duy, > > On Mon, 6 Jun 2016, Nguyễn Thái Ngọc Duy wrote: > >> diff --git a/diff.h b/diff.h >> index b497078..9e42556 100644 >> --- a/diff.h >> +++ b/diff.h >> @@ -92,6 +92,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct >> diff_options *opt, void *data) >> #define DIFF_OPT_FUNCCONTEXT (1 << 29) >> #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30) >> #define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31) >> +#define DIFF_OPT_SHIFT_INTENT_TO_ADD (1UL << 32) > > I am afraid that this is not enough. On Windows, sizeof(unsigned long) is > 4 (and it is perfectly legal). That means that you would have to use at > least (1ULL << 32), OK. > but then you also have to change the type of xdl_opts > to uint64_t, which in turn means that you will have to change the > definition of xpparam_t's "flags" field from unsigned long to uint64_t. I miss a connection here. This new flag is intended to be used in "flags" field in struct diff_options. Is there any chance it can be set on xdl_opts (of the same struct, I assume)? > Maybe this can be avoided? I don't see a good way to avoid it. We normally enable or disable diff features as bit flags and now we run out of bits. Adding something like "flags2" works, but not pretty. Any suggestion is welcome. -- Duy
Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
Hi Duy, On Mon, 6 Jun 2016, Nguyễn Thái Ngọc Duy wrote: > diff --git a/diff.h b/diff.h > index b497078..9e42556 100644 > --- a/diff.h > +++ b/diff.h > @@ -92,6 +92,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct > diff_options *opt, void *data) > #define DIFF_OPT_FUNCCONTEXT (1 << 29) > #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30) > #define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31) > +#define DIFF_OPT_SHIFT_INTENT_TO_ADD (1UL << 32) I am afraid that this is not enough. On Windows, sizeof(unsigned long) is 4 (and it is perfectly legal). That means that you would have to use at least (1ULL << 32), but then you also have to change the type of xdl_opts to uint64_t, which in turn means that you will have to change the definition of xpparam_t's "flags" field from unsigned long to uint64_t. Maybe this can be avoided? Ciao, Johannes
Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
On Tue, Jun 7, 2016 at 3:42 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> +--shift-ita:: >> + By default entries added by "git add -N" appear as an existing >> + empty file in "git diff" and a new file in "git diff --cached". >> + This option makes the entry appear as a new file in "git diff" >> + and non-existent in "git diff --cached". > > I do not think this should exist at the UI level, I need it. I do "git diff --stat" and "git diff --cached --stat" a lot more often than "git status". Without this option, I'm stuck with the old behavior. > even though the > use of it in wt-status.c (below) makes a very good sense at least > as a temporary band-aid. > > At the philosophical level, I however think this "I-T-A does not > logically exist in the index (yet)" is a mistake, and "an option > controls if I-T-A does or does not exist depending on who calls it" > is even worse; it is a road to insanity. i-t-a entries have dual personality (perhaps because it's implemented as an index entry). Although I think the "does not exist" aspect will win in most cases. The intention behind the revert is we have more time to examine case by case and gradually convert them. Maybe in the end one behavior wins and we no longer need another. A thought of keeping i-t-a entries in an index extension instead crossed my mind. It may simplify things a bit (e.g. there's no "ghost" entries any more and active_nr in 3/4 can remain "the number of _real_ entries"). The few parts that do need to know about i-t-a entries need explict modification (probably git-reset and git-diff). But I don't know yet if it would just lead to another nightmare. > For example, because I-T-A does not logically exist in the index, > "git reset --hard" should not remove it but make it untracked again > (but I do not think it does). After "git add -N foo", because "foo" > does not exist in the index, "git clean" should remove it for the > definition of what's in the index to be logically consistent, but > the whole intent of "add -N" is that the user meant it is worth > checking into sometime in the future, which contradicts with its > removal upon "clean". I think we should fix them. I started that and so far only 4d55200 (grep: make it clear i-t-a entries are ignored - 2015-12-27) has made it to 'master'. > So, I dunno. I just remembered why the old behavior (abort to commit if i-t-a entries are present) bugged me: it does not work well with splitting changes in worktree into multiple commits (e.g. with "git add -p"). Even though I want git remind me to commit an i-t-a entry in the end, it does not necessarily mean I have to do it in the next commit, which may cover a bunch of files except that i-t-a file. I don't see any way around that except ignoring i-t-a entries at commit time. If there's another way, I'm all ears. -- 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/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
Nguyễn Thái Ngọc Duy writes: > +--shift-ita:: > + By default entries added by "git add -N" appear as an existing > + empty file in "git diff" and a new file in "git diff --cached". > + This option makes the entry appear as a new file in "git diff" > + and non-existent in "git diff --cached". I do not think this should exist at the UI level, even though the use of it in wt-status.c (below) makes a very good sense at least as a temporary band-aid. At the philosophical level, I however think this "I-T-A does not logically exist in the index (yet)" is a mistake, and "an option controls if I-T-A does or does not exist depending on who calls it" is even worse; it is a road to insanity. For example, because I-T-A does not logically exist in the index, "git reset --hard" should not remove it but make it untracked again (but I do not think it does). After "git add -N foo", because "foo" does not exist in the index, "git clean" should remove it for the definition of what's in the index to be logically consistent, but the whole intent of "add -N" is that the user meant it is worth checking into sometime in the future, which contradicts with its removal upon "clean". So, I dunno. -- 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
