*Re: [PATCH 2/4] t3701: add failing test for pathological context lines
2018-02-13 10:44 ` [PATCH 2/4] t3701: add failing test for pathological context lines Phillip Wood
@ 2018-02-13 20:35 ` Junio C Hamano0 siblings, 0 replies; 79+ messages in thread
From: Junio C Hamano @ 2018-02-13 20:35 UTC (permalink / raw)
To: Phillip Wood; +Cc: Git Mailing List, Phillip Wood
Phillip Wood <phillip.wood@talktalk.net> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When a hunk is skipped by add -i the offsets of subsequent hunks are
> not adjusted to account for any missing insertions due to the skipped
> hunk. Most of the time this does not matter as apply uses the context
> lines to apply the subsequent hunks in the correct place, however in
> pathological cases the context lines will match at the now incorrect
> offset and the hunk will be applied in the wrong place. The offsets of
Good. The --recount "feature" on the receiving end does not have
enough information to do a job as good as the code sitting at the
side of producing a patch to be applied, and this goes in the right
direction.
^permalinkrawreply [flat|nested] 79+ messages in thread

*Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values
2018-02-20 17:39 ` Junio C Hamano@ 2018-02-21 11:42 ` Phillip Wood
2018-02-21 16:58 ` Junio C Hamano0 siblings, 1 reply; 79+ messages in thread
From: Phillip Wood @ 2018-02-21 11:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Brian M. Carlson, Phillip Wood
On 20/02/18 17:39, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Purge the index lines from diffs so we're not hard coding sha1 hash
>> values in the expected output.
>
> The motivation of this patch is clear, but all-zero object name for
> missing side of deletion or creation patch should not change when we
> transition to any hash function. Neither the permission bits shown
> in the output (and whether the index line has the bits are shown on
> it in the first place, i.e. the index line of a creation patch does
> not, whilethe one in a modification patch does).
>
> So I am a bit ambivalent about this change.
>
> Perhaps have a filter that redacts, instead of removes, selected
> pieces of information that are likely to change while hash
> transition, and use that consistently to filter both the expected
> output and the actual output before comparing?
>
Keeping the permission bits makes sense (I'd not thought of them when I
created the patch) as we want to check that the file has the correct
permissions. As for the all-zero object name, is it really worth leaving
it in - if a file has been created or deleted then we'll still have
/dev/null as the file name for one side or the other and the diff lines
will show it as well. As these tests are just to check the state of the
index then I'm not sure the hash values add anything. How do you feel
about a filter like
sed "/^ index/s/ [0-9a-f][0-9a-f]*\.\.[0-9a-f][0-9a-f]*/x/"
Best Wishes
Phillip
^permalinkrawreply [flat|nested] 79+ messages in thread

*Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values
2018-02-21 11:42 ` Phillip Wood@ 2018-02-21 16:58 ` Junio C Hamano0 siblings, 0 replies; 79+ messages in thread
From: Junio C Hamano @ 2018-02-21 16:58 UTC (permalink / raw)
To: Phillip Wood; +Cc: Git Mailing List, Brian M. Carlson, Phillip Wood
Phillip Wood <phillip.wood@talktalk.net> writes:
> Keeping the permission bits makes sense (I'd not thought of them when
> I created the patch) as we want to check that the file has the correct
> permissions. As for the all-zero object name, is it really worth
> leaving it in - if a file has been created or deleted then we'll still
> have /dev/null as the file name for one side or the other and the diff
> lines will show it as well. As these tests are just to check the state
> of the index then I'm not sure the hash values add anything. How do
> you feel about a filter like
>
> sed "/^ index/s/ [0-9a-f][0-9a-f]*\.\.[0-9a-f][0-9a-f]*/x/"
Something like that, with perhaps the single 'x' replaced with
something more normal looking and the all-zero thing special cased,
was what I had in mind.
Special casing all-zero matters. I know that the current code uses
all-zero on the missing side. The thing is, tests are about
protecting the correctness we currently have, and we want to catch
the next idiot that breaks the code to stop showing all-zero when
talking about the missing side.
^permalinkrawreply [flat|nested] 79+ messages in thread

*Re: [PATCH v3 2/9] t3701: indent here documents
2018-02-28 11:00 ` Phillip Wood@ 2018-02-28 15:37 ` Junio C Hamano
2018-03-01 10:57 ` Phillip Wood0 siblings, 1 reply; 79+ messages in thread
From: Junio C Hamano @ 2018-02-28 15:37 UTC (permalink / raw)
To: Phillip Wood
Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood
Phillip Wood <phillip.wood@talktalk.net> writes:
> Is there an easy way for contributors to compare the branch they post to
> what ends up it pu?
Distributed work is pretty much symmetric, so it can be done the
same way as one would review a rerolled series by another co-worker.
$ git log --oneline --first-parent origin/master..origin/pu
would show merges of topic branches, so you can find the tip of the
topic of your earlier submission (it would be one $commit^2; call
that $topic). origin/master..$topic would be the one branch
(i.e. what is in 'pu') to be compared.
The other branch to be compared is what you sent the previous one
out of, or the new version of the patches.
To compare two branches, git://github.com/trast/tbdiff is one of the
easier way.
Before I learned about the tool, I used to "format-patch --stdout"
on both branches, and ran "diff -u" between them, as a crude measure;
it was more useful for spotting typofixes in the log messages than
code changes, before I got good at reading diff of diffs ;-).
Also, tentatively rebasing the two branches on a common base, and
then doing "git diff $oldtopic~$N $newtopic~$N" or something like
that for varying value of $N (and N==0 is a good way for final
sanity checks).
^permalinkrawreply [flat|nested] 79+ messages in thread

*Re: [PATCH v3 2/9] t3701: indent here documents
2018-02-28 15:37 ` Junio C Hamano@ 2018-03-01 10:57 ` Phillip Wood
2018-03-01 15:58 ` Junio C Hamano0 siblings, 1 reply; 79+ messages in thread
From: Phillip Wood @ 2018-03-01 10:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood
Hi Junio
On 28/02/18 15:37, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
>
>> Is there an easy way for contributors to compare the branch they post to
>> what ends up it pu?
>
> Distributed work is pretty much symmetric, so it can be done the
> same way as one would review a rerolled series by another co-worker.
>
> $ git log --oneline --first-parent origin/master..origin/pu
>
> would show merges of topic branches, so you can find the tip of the
> topic of your earlier submission (it would be one $commit^2; call
> that $topic). origin/master..$topic would be the one branch
> (i.e. what is in 'pu') to be compared.
>
> The other branch to be compared is what you sent the previous one
> out of, or the new version of the patches.
>
> To compare two branches, git://github.com/trast/tbdiff is one of the
> easier way.
>
> Before I learned about the tool, I used to "format-patch --stdout"
> on both branches, and ran "diff -u" between them, as a crude measure;
> it was more useful for spotting typofixes in the log messages than
> code changes, before I got good at reading diff of diffs ;-).
>
> Also, tentatively rebasing the two branches on a common base, and
> then doing "git diff $oldtopic~$N $newtopic~$N" or something like
> that for varying value of $N (and N==0 is a good way for final
> sanity checks).
Thanks for the tips, tbdiff looks useful (I just need to learn to read
diffs of diffs!). I also find rebasing them on a common ancestor useful
but its a bit tedious.
Thanks again
Phillip
^permalinkrawreply [flat|nested] 79+ messages in thread

*Re: [PATCH v3 2/9] t3701: indent here documents
2018-03-01 10:57 ` Phillip Wood@ 2018-03-01 15:58 ` Junio C Hamano0 siblings, 0 replies; 79+ messages in thread
From: Junio C Hamano @ 2018-03-01 15:58 UTC (permalink / raw)
To: Phillip Wood
Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood
Phillip Wood <phillip.wood@talktalk.net> writes:
> Thanks for the tips, tbdiff looks useful (I just need to learn to read
> diffs of diffs!). I also find rebasing them on a common ancestor useful
> but its a bit tedious.
Yes, comparing two versions of a series is somewhat unusual and
needs getting used to before one can do so efficiently. You do
not have to always rebase (tbdiff is fairly good at what it does
even when two ranges are far apart), but it helps not to begin
developing on a codebase that is newer than needed (e.g. a bugfix
on 'next' is unneeded unless you are fixing a bug introduced by a
topic that is still on 'next'---in which case the best fix is one
that is on that topic, without depending on the rest of 'next').
In any case, thank _you_ for contributing.
^permalinkrawreply [flat|nested] 79+ messages in thread

*Re: [PATCH v4 4/9] t3701: don't hard code sha1 hash values
2018-03-02 15:55 ` SZEDER Gábor@ 2018-03-02 16:09 ` Junio C Hamano
2018-03-05 10:59 ` Phillip Wood0 siblings, 1 reply; 79+ messages in thread
From: Junio C Hamano @ 2018-03-02 16:09 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Phillip Wood, Git Mailing List, Brian M. Carlson, Eric Sunshine,
Phillip Wood
SZEDER Gábor <szeder.dev@gmail.com> writes:
>> +diff_cmp () {
>> + for x
>> + do
>> + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \
>> + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \
>> + -e '/^index/s/ 00*\.\./ 0000000../' \
>> + -e '/^index/s/\.\.00*$/..0000000/' \
>> + -e '/^index/s/\.\.00* /..0000000 /' \
>> + "$x" >"$x.filtered"
>> + done
>> + test_cmp "$1.filtered" "$2.filtered"
>> +}
>
> t3701 is not the only test script comparing diffs, many other
> tests do so:
>
> $ git grep 'diff --git' t/ |wc -l
> 835
>
> It's totally inaccurate, but a good ballpark figure.
>
> I think this function should be a widely available test helper
> function in 'test-lib-functions.sh', perhaps renamed to
> 'test_cmp_diff', so those other tests could use it as well.
I am on the fence.
The above does not redact enough (e.g. rename/copy score should be
redacted while comparing) to serve as a generic filter.
We could certainly extend it when we make code in other tests use
the helper, but then we can do the moving to test-lib-functions when
that happens, too.
Those who will be writing new tests that need to compare two diff
outputs are either (1) people who are careful and try to find
existing tests that do the same, to locate the helper we already
have to be reused in theirs, or (2) people who don't and roll their
own. The latter camp cannot be helped either way, but the former
will run "git grep 'diff --git' t/" and find the helper whether it
is in this script or in test-lib-functions, I would think.
So, I certainly do not mind a reroll to move it to a more generic
place, but I do not think I would terribly mind if we leave it in
its current place, later to be moved by the first new caller that
wants to use it from outside this script.
^permalinkrawreply [flat|nested] 79+ messages in thread

*Re: [PATCH v4 4/9] t3701: don't hard code sha1 hash values
2018-03-02 16:09 ` Junio C Hamano@ 2018-03-05 10:59 ` Phillip Wood
2018-03-05 12:32 ` SZEDER Gábor0 siblings, 1 reply; 79+ messages in thread
From: Phillip Wood @ 2018-03-05 10:59 UTC (permalink / raw)
To: Junio C Hamano, SZEDER Gábor
Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood
On 02/03/18 16:09, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>>> +diff_cmp () {
>>> + for x
>>> + do
>>> + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \
>>> + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \
>>> + -e '/^index/s/ 00*\.\./ 0000000../' \
>>> + -e '/^index/s/\.\.00*$/..0000000/' \
>>> + -e '/^index/s/\.\.00* /..0000000 /' \
>>> + "$x" >"$x.filtered"
>>> + done
>>> + test_cmp "$1.filtered" "$2.filtered"
>>> +}
>>
>> t3701 is not the only test script comparing diffs, many other
>> tests do so:
>>
>> $ git grep 'diff --git' t/ |wc -l
>> 835
>>
>> It's totally inaccurate, but a good ballpark figure.
>>
>> I think this function should be a widely available test helper
>> function in 'test-lib-functions.sh', perhaps renamed to
>> 'test_cmp_diff', so those other tests could use it as well.
>
> I am on the fence.
>
> The above does not redact enough (e.g. rename/copy score should be
> redacted while comparing) to serve as a generic filter.
>
> We could certainly extend it when we make code in other tests use
> the helper, but then we can do the moving to test-lib-functions when
> that happens, too.
>
> Those who will be writing new tests that need to compare two diff
> outputs are either (1) people who are careful and try to find
> existing tests that do the same, to locate the helper we already
> have to be reused in theirs, or (2) people who don't and roll their
> own. The latter camp cannot be helped either way, but the former
> will run "git grep 'diff --git' t/" and find the helper whether it
> is in this script or in test-lib-functions, I would think.
>
> So, I certainly do not mind a reroll to move it to a more generic
> place, but I do not think I would terribly mind if we leave it in
> its current place, later to be moved by the first new caller that
> wants to use it from outside this script.
>
I did wonder about putting this function in a library when I first wrote
it but decided to wait and see if it is useful instead. As Junio points
out it would need to be improved to act as a generic filter, so I'll
take the easy option and leave it where it is at the moment.
Best Wishes
Phillip
^permalinkrawreply [flat|nested] 79+ messages in thread

*Re: [PATCH v4 4/9] t3701: don't hard code sha1 hash values
2018-03-05 10:59 ` Phillip Wood@ 2018-03-05 12:32 ` SZEDER Gábor0 siblings, 0 replies; 79+ messages in thread
From: SZEDER Gábor @ 2018-03-05 12:32 UTC (permalink / raw)
To: Phillip Wood
Cc: Junio C Hamano, Git Mailing List, Brian M. Carlson, Eric Sunshine
On Mon, Mar 5, 2018 at 11:59 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
> I did wonder about putting this function in a library when I first wrote
> it but decided to wait and see if it is useful instead. As Junio points
> out it would need to be improved to act as a generic filter, so I'll
> take the easy option and leave it where it is at the moment.
Makes sense. I just pointed it out, because I could have used it
already in its current form for a test I was working on last week.
^permalinkrawreply [flat|nested] 79+ messages in thread

*Re: [PATCH v5 0/9] Correct offsets of hunks when one is skipped
2018-03-05 18:50 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Junio C Hamano
@ 2018-03-06 10:25 ` Phillip Wood0 siblings, 0 replies; 79+ messages in thread
From: Phillip Wood @ 2018-03-06 10:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood
On 05/03/18 18:50, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> I've updated these to clean up the perl style in response to Junio's
>> comments on v4.
>
> I've considered the use of "apply --recount" in this script (eh,
> rather, the existence of that option in "apply" command itself ;-))
> a bug that need to be eventually fixed for a long time, so the copy
> of earlier rounds I've been carrying in my tree were forked from
> 'maint'.
Oh I've been basing this on master, I hope that wasn't a problem.
> I'll queue this round on the same base commit as before to replace;
> I _think_ this is ready for 'next'.
Yes I think so (I've not come up with any new ways to break it, lets see
if someone else can)
> Thanks for working on this.
>
Thanks, I use add -p quite a lot so it's nice to be able to contribute
something back.
Best Wishes
Phillip
^permalinkrawreply [flat|nested] 79+ messages in thread