| Message ID | 20200808075323.36041-1-ray@ameretat.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] diff-lib: use worktree mode in diffs from i-t-a entries | expand |
On Sat, 8 Aug 2020 at 09:55, Raymond E. Pasco <ray@ameretat.dev> wrote: > > When creating "new file" diffs against i-t-a index entries, diff-lib > erroneously used the mode of the cache entry rather than the mode of the > file in the worktree. This changes run_diff_files() to correctly use the > mode of the worktree file in this case. Good catch! Describing the current state of affairs and using imperative mode, it could be something like: When creating "new file" diffs against i-t-a index entries, diff-lib erroneously uses the mode of the cache entry rather than the mode of the file in the worktree. Change run_diff_files() to correctly use the mode of the worktree file in this case. More importantly: I can confirm that the bug is there before your patch and that your patch fixes it. Could you add a test in this patch so we can trust that this stays fixed? Martin
On Sat Aug 8, 2020 at 4:48 AM EDT, Martin Ågren wrote: > Describing the current state of affairs and using imperative mode, it > could be something like: > > When creating "new file" diffs against i-t-a index entries, diff-lib > erroneously uses the mode of the cache entry rather than the mode of > the file in the worktree. Change run_diff_files() to correctly use the > mode of the worktree file in this case. I see both styles around in the tree (past for the state of the world before this patch, present for the state of the world as of this patch, vs. present for the state of the world just before and just after applying this patch). Neither is unreadable to me so I just want to do whatever's the standard around here. (I'm not convinced, as a matter of grammar, that the commit-message-present verb form is really in the imperative mood; I think the freeform nature of English grammar obscures that it's the present active infinitive, analogous to, say, the fact that a French software program with an "open file" button will say "ouvrir" and not "ouvrez".) > I can confirm that the bug is there before your patch and that your > patch fixes it. Could you add a test in this patch so we can trust that > this stays fixed? The whole set of i-t-a diffing behaviors needs a test suite (unless I've grepped very poorly), which will come in another patchset. t4140 in this thread's main patchset assumes they work.
On Sat, 8 Aug 2020 at 13:24, Raymond E. Pasco <ray@ameretat.dev> wrote: > > On Sat Aug 8, 2020 at 4:48 AM EDT, Martin Ågren wrote: > > Describing the current state of affairs and using imperative mode, it > > could be something like: > > > > When creating "new file" diffs against i-t-a index entries, diff-lib > > erroneously uses the mode of the cache entry rather than the mode of > > the file in the worktree. Change run_diff_files() to correctly use the > > mode of the worktree file in this case. > > I see both styles around in the tree (past for the state of the world > before this patch, present for the state of the world as of this patch, > vs. present for the state of the world just before and just after > applying this patch). Neither is unreadable to me so I just want to do > whatever's the standard around here. Yeah, there are all kinds of log messages in the history. SubmittingPatches (search for "imperative") recommends this way of writing. > (I'm not convinced, as a matter of grammar, that the > commit-message-present verb form is really in the imperative mood; I > think the freeform nature of English grammar obscures that it's the > present active infinitive, analogous to, say, the fact that a French > software program with an "open file" button will say "ouvrir" and not > "ouvrez".) When you put it that way, I'm also not sure. :-) > The whole set of i-t-a diffing behaviors needs a test suite (unless I've > grepped very poorly), which will come in another patchset. t4140 in this > thread's main patchset assumes they work. I did grep a little when I wrote my previous reply and I didn't find anything either. I guess you could add a very small testcase here and then base that future series on top of this commit. Or in lieu of a test, maybe this could be used: Tested-by: Martin Ågren <martin.agren@gmail.com> Martin
"Raymond E. Pasco" <ray@ameretat.dev> writes: > When creating "new file" diffs against i-t-a index entries, diff-lib > erroneously used the mode of the cache entry rather than the mode of the > file in the worktree. This changes run_diff_files() to correctly use the > mode of the worktree file in this case. > > Signed-off-by: Raymond E. Pasco <ray@ameretat.dev> > --- > This is a distinct bugfix from the other changes, so I've sent it as a > separate patch also based on v2.28.0. > > diff-lib.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/diff-lib.c b/diff-lib.c > index 25fd2dee19..50521e2093 100644 > --- a/diff-lib.c > +++ b/diff-lib.c > @@ -219,7 +219,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option) > continue; > } else if (revs->diffopt.ita_invisible_in_index && > ce_intent_to_add(ce)) { > - diff_addremove(&revs->diffopt, '+', ce->ce_mode, > + newmode = ce_mode_from_stat(ce, st.st_mode); > + diff_addremove(&revs->diffopt, '+', newmode, > &null_oid, 0, ce->name, 0); ;-) The ita-invisible-in-index option means that Git must ignore anything in the index about the entry, other than just the fact that the path is subject to comparison, so use of ce->ce_mode here is wrong by definition. Makes sense. > continue; > }
diff --git a/diff-lib.c b/diff-lib.c index 25fd2dee19..50521e2093 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -219,7 +219,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option) continue; } else if (revs->diffopt.ita_invisible_in_index && ce_intent_to_add(ce)) { - diff_addremove(&revs->diffopt, '+', ce->ce_mode, + newmode = ce_mode_from_stat(ce, st.st_mode); + diff_addremove(&revs->diffopt, '+', newmode, &null_oid, 0, ce->name, 0); continue; }
When creating "new file" diffs against i-t-a index entries, diff-lib erroneously used the mode of the cache entry rather than the mode of the file in the worktree. This changes run_diff_files() to correctly use the mode of the worktree file in this case. Signed-off-by: Raymond E. Pasco <ray@ameretat.dev> --- This is a distinct bugfix from the other changes, so I've sent it as a separate patch also based on v2.28.0. diff-lib.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)