* [PATCH 1/2] format-patch: Inform user that patch-id generation is unstable @ 2019-04-26 23:51 Stephen Boyd 2019-04-26 23:51 ` [PATCH 2/2] format-patch: Make --base patch-id output stable Stephen Boyd 0 siblings, 1 reply; 5+ messages in thread From: Stephen Boyd @ 2019-04-26 23:51 UTC (permalink / raw) To: git; +Cc: Xiaolong Ye I tried out 'git format-patch --base' with a set of commits that modifies more than one file. It turns out that the way this command is implemented it actually uses the unstable version of patch-id instead of the stable version that's documented. When I tried to modify the existing test to use 'git patch-id --stable' vs. 'git patch-id --unstable' I found that it didn't matter, the test still passed. Let's expand on the test here so it is a little more complicated and then use that to show that the patch-id generation is actually unstable vs. stable. Update the documentation as well. Cc: Xiaolong Ye <xiaolong.ye@intel.com> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- Documentation/git-format-patch.txt | 2 +- t/t4014-format-patch.sh | 36 +++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 1af85d404f51..e8cc792e7f5d 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -583,7 +583,7 @@ of 'base commit' in topological order before the patches can be applied. The 'base commit' is shown as "base-commit: " followed by the 40-hex of the commit object name. A 'prerequisite patch' is shown as "prerequisite-patch-id: " followed by the 40-hex 'patch id', which can -be obtained by passing the patch through the `git patch-id --stable` +be obtained by passing the patch through the `git patch-id --unstable` command. Imagine that on top of the public commit P, you applied well-known diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index b6e2fdbc4410..e82c6c7d9177 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -36,8 +36,27 @@ test_expect_success setup ' git checkout master && git diff-tree -p C2 | git apply --index && test_tick && - git commit -m "Master accepts moral equivalent of #2" + git commit -m "Master accepts moral equivalent of #2" && + git checkout side && + git checkout -b patchid && + for i in 5 6 1 2 3 A 4 B C 7 8 9 10 D E F; do echo "$i"; done >file2 && + for i in 1 2 3 A 4 B C 7 8 9 10 D E F 5 6; do echo "$i"; done >file3 && + for i in 8 9 10; do echo "$i"; done >file && + git add file file2 file3 && + test_tick && + git commit -m "patchid 1" && + for i in 4 A B 7 8 9 10; do echo "$i"; done >file2 && + for i in 8 9 10 5 6; do echo "$i"; done >file3 && + git add file2 file3 && + test_tick && + git commit -m "patchid 2" && + for i in 10 5 6; do echo "$i"; done >file && + git add file && + test_tick && + git commit -m "patchid 3" && + + git checkout master ' test_expect_success "format-patch --ignore-if-in-upstream" ' @@ -1559,16 +1578,23 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' ' ' test_expect_success 'format-patch --base' ' - git checkout side && + git checkout patchid && git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual1 && git format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 && echo >expected && echo "base-commit: $(git rev-parse HEAD~3)" >>expected && - echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>expected && - echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>expected && + echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk "{print \$1}")" >>expected && + echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk "{print \$1}")" >>expected && signature >> expected && test_cmp expected actual1 && - test_cmp expected actual2 + test_cmp expected actual2 && + echo >fail && + echo "base-commit: $(git rev-parse HEAD~3)" >>fail && + echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>fail && + echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>fail && + signature >> fail && + ! test_cmp fail actual1 && + ! test_cmp fail actual2 ' test_expect_success 'format-patch --base errors out when base commit is in revision list' ' -- Sent by a computer through tubes ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] format-patch: Make --base patch-id output stable 2019-04-26 23:51 [PATCH 1/2] format-patch: Inform user that patch-id generation is unstable Stephen Boyd @ 2019-04-26 23:51 ` Stephen Boyd 2019-05-07 4:38 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Stephen Boyd @ 2019-04-26 23:51 UTC (permalink / raw) To: git; +Cc: Xiaolong Ye We weren't flushing the context each time we processed a hunk in the patch-id generation code in diff.c, but we were doing that when we generated "stable" patch-ids with the 'patch-id' tool. Let's port that similar logic over from patch-id.c into diff.c so we can get the same hash when we're generating patch-ids for 'format-patch --base=' types of command invocations. Cc: Xiaolong Ye <xiaolong.ye@intel.com> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- I wonder if we need to make some other sort of form of "prerequisite-patch-id:" here and let that be a legacy form of the patch-id so that users know that they have a fixed version of this code? Maybe "prerequisite-stable-patch-id:"? Or we don't have to care because it's been broken for anything besides the most trivial type of patches and presumably users aren't able to use it with 'patch-id --stable'? Documentation/git-format-patch.txt | 2 +- builtin/log.c | 2 +- builtin/patch-id.c | 17 +--------------- diff.c | 32 +++++++++++++++++++++++++----- diff.h | 3 ++- patch-ids.c | 10 +++++----- patch-ids.h | 2 +- t/t4014-format-patch.sh | 8 ++++---- 8 files changed, 42 insertions(+), 34 deletions(-) diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index e8cc792e7f5d..1af85d404f51 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -583,7 +583,7 @@ of 'base commit' in topological order before the patches can be applied. The 'base commit' is shown as "base-commit: " followed by the 40-hex of the commit object name. A 'prerequisite patch' is shown as "prerequisite-patch-id: " followed by the 40-hex 'patch id', which can -be obtained by passing the patch through the `git patch-id --unstable` +be obtained by passing the patch through the `git patch-id --stable` command. Imagine that on top of the public commit P, you applied well-known diff --git a/builtin/log.c b/builtin/log.c index e43ee12fb1dd..147850dc7317 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1435,7 +1435,7 @@ static void prepare_bases(struct base_tree_info *bases, struct object_id *patch_id; if (*commit_base_at(&commit_base, commit)) continue; - if (commit_patch_id(commit, &diffopt, &oid, 0)) + if (commit_patch_id(commit, &diffopt, &oid, 0, 1)) die(_("cannot get patch id")); ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id); patch_id = bases->patch_id + bases->nr_patch_id; diff --git a/builtin/patch-id.c b/builtin/patch-id.c index 970d0d30b4f4..bd28b80b2d0f 100644 --- a/builtin/patch-id.c +++ b/builtin/patch-id.c @@ -1,5 +1,6 @@ #include "builtin.h" #include "config.h" +#include "diff.h" static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result) { @@ -54,22 +55,6 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after) return 1; } -static void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx) -{ - unsigned char hash[GIT_MAX_RAWSZ]; - unsigned short carry = 0; - int i; - - git_SHA1_Final(hash, ctx); - git_SHA1_Init(ctx); - /* 20-byte sum, with carry */ - for (i = 0; i < GIT_SHA1_RAWSZ; ++i) { - carry += result->hash[i] + hash[i]; - result->hash[i] = carry; - carry >>= 8; - } -} - static int get_one_patchid(struct object_id *next_oid, struct object_id *result, struct strbuf *line_buf, int stable) { diff --git a/diff.c b/diff.c index 4d3cf83a27e5..8e3675b416a2 100644 --- a/diff.c +++ b/diff.c @@ -5988,6 +5988,22 @@ static int remove_space(char *line, int len) return dst - line; } +void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx) +{ + unsigned char hash[GIT_MAX_RAWSZ]; + unsigned short carry = 0; + int i; + + git_SHA1_Final(hash, ctx); + git_SHA1_Init(ctx); + /* 20-byte sum, with carry */ + for (i = 0; i < GIT_SHA1_RAWSZ; ++i) { + carry += result->hash[i] + hash[i]; + result->hash[i] = carry; + carry >>= 8; + } +} + static void patch_id_consume(void *priv, char *line, unsigned long len) { struct patch_id_t *data = priv; @@ -6012,8 +6028,8 @@ static void patch_id_add_mode(git_SHA_CTX *ctx, unsigned mode) git_SHA1_Update(ctx, buf, len); } -/* returns 0 upon success, and writes result into sha1 */ -static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only) +/* returns 0 upon success, and writes result into oid */ +static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only, int stable) { struct diff_queue_struct *q = &diff_queued_diff; int i; @@ -6023,6 +6039,7 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid git_SHA1_Init(&ctx); memset(&data, 0, sizeof(struct patch_id_t)); data.ctx = &ctx; + oidclr(oid); for (i = 0; i < q->nr; i++) { xpparam_t xpp; @@ -6098,17 +6115,22 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid patch_id_consume, &data, &xpp, &xecfg)) return error("unable to generate patch-id diff for %s", p->one->path); + + if (stable) + flush_one_hunk(oid, &ctx); } - git_SHA1_Final(oid->hash, &ctx); + if (!stable) + git_SHA1_Final(oid->hash, &ctx); + return 0; } -int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only) +int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only, int stable) { struct diff_queue_struct *q = &diff_queued_diff; int i; - int result = diff_get_patch_id(options, oid, diff_header_only); + int result = diff_get_patch_id(options, oid, diff_header_only, stable); for (i = 0; i < q->nr; i++) diff_free_filepair(q->queue[i]); diff --git a/diff.h b/diff.h index b20cbcc09142..de24a048649e 100644 --- a/diff.h +++ b/diff.h @@ -436,7 +436,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option); int run_diff_index(struct rev_info *revs, int cached); int do_diff_cache(const struct object_id *, struct diff_options *); -int diff_flush_patch_id(struct diff_options *, struct object_id *, int); +int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int); +void flush_one_hunk(struct object_id *, git_SHA_CTX *); int diff_result_code(struct diff_options *, int); diff --git a/patch-ids.c b/patch-ids.c index c262e1be9c9c..f70d3966542d 100644 --- a/patch-ids.c +++ b/patch-ids.c @@ -11,7 +11,7 @@ static int patch_id_defined(struct commit *commit) } int commit_patch_id(struct commit *commit, struct diff_options *options, - struct object_id *oid, int diff_header_only) + struct object_id *oid, int diff_header_only, int stable) { if (!patch_id_defined(commit)) return -1; @@ -22,7 +22,7 @@ int commit_patch_id(struct commit *commit, struct diff_options *options, else diff_root_tree_oid(&commit->object.oid, "", options); diffcore_std(options); - return diff_flush_patch_id(options, oid, diff_header_only); + return diff_flush_patch_id(options, oid, diff_header_only, stable); } /* @@ -46,11 +46,11 @@ static int patch_id_neq(const void *cmpfn_data, struct patch_id *b = (void *)entry_or_key; if (is_null_oid(&a->patch_id) && - commit_patch_id(a->commit, opt, &a->patch_id, 0)) + commit_patch_id(a->commit, opt, &a->patch_id, 0, 0)) return error("Could not get patch ID for %s", oid_to_hex(&a->commit->object.oid)); if (is_null_oid(&b->patch_id) && - commit_patch_id(b->commit, opt, &b->patch_id, 0)) + commit_patch_id(b->commit, opt, &b->patch_id, 0, 0)) return error("Could not get patch ID for %s", oid_to_hex(&b->commit->object.oid)); return !oideq(&a->patch_id, &b->patch_id); @@ -80,7 +80,7 @@ static int init_patch_id_entry(struct patch_id *patch, struct object_id header_only_patch_id; patch->commit = commit; - if (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1)) + if (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1, 0)) return -1; hashmap_entry_init(patch, sha1hash(header_only_patch_id.hash)); diff --git a/patch-ids.h b/patch-ids.h index 82a12b66f889..03bb04e7071f 100644 --- a/patch-ids.h +++ b/patch-ids.h @@ -20,7 +20,7 @@ struct patch_ids { }; int commit_patch_id(struct commit *commit, struct diff_options *options, - struct object_id *oid, int); + struct object_id *oid, int, int); int init_patch_ids(struct repository *, struct patch_ids *); int free_patch_ids(struct patch_ids *); struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *); diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index e82c6c7d9177..7a5833a4b275 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1583,15 +1583,15 @@ test_expect_success 'format-patch --base' ' git format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 && echo >expected && echo "base-commit: $(git rev-parse HEAD~3)" >>expected && - echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk "{print \$1}")" >>expected && - echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk "{print \$1}")" >>expected && + echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>expected && + echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>expected && signature >> expected && test_cmp expected actual1 && test_cmp expected actual2 && echo >fail && echo "base-commit: $(git rev-parse HEAD~3)" >>fail && - echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>fail && - echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>fail && + echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk "{print \$1}")" >>fail && + echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk "{print \$1}")" >>fail && signature >> fail && ! test_cmp fail actual1 && ! test_cmp fail actual2 -- Sent by a computer through tubes ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] format-patch: Make --base patch-id output stable 2019-04-26 23:51 ` [PATCH 2/2] format-patch: Make --base patch-id output stable Stephen Boyd @ 2019-05-07 4:38 ` Junio C Hamano 2019-05-07 17:46 ` Stephen Boyd 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2019-05-07 4:38 UTC (permalink / raw) To: Stephen Boyd; +Cc: git, Xiaolong Ye Stephen Boyd <sboyd@kernel.org> writes: > I wonder if we need to make some other sort of form of > "prerequisite-patch-id:" here and let that be a legacy form of the > patch-id so that users know that they have a fixed version of this code? > Maybe "prerequisite-stable-patch-id:"? Or we don't have to care because > it's been broken for anything besides the most trivial type of patches > and presumably users aren't able to use it with 'patch-id --stable'? Do projects actively use -O<orderfile> when generating the patches? I had an impression that not many do, and without -O<orderfile> in the picture, --unstable/--stable would not matter, no? So, I am not sure if this matters very much in practice. > > Documentation/git-format-patch.txt | 2 +- > builtin/log.c | 2 +- > builtin/patch-id.c | 17 +--------------- > diff.c | 32 +++++++++++++++++++++++++----- > diff.h | 3 ++- > patch-ids.c | 10 +++++----- > patch-ids.h | 2 +- > t/t4014-format-patch.sh | 8 ++++---- > 8 files changed, 42 insertions(+), 34 deletions(-) > > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > index e8cc792e7f5d..1af85d404f51 100644 > --- a/Documentation/git-format-patch.txt > +++ b/Documentation/git-format-patch.txt > @@ -583,7 +583,7 @@ of 'base commit' in topological order before the patches can be applied. > The 'base commit' is shown as "base-commit: " followed by the 40-hex of > the commit object name. A 'prerequisite patch' is shown as > "prerequisite-patch-id: " followed by the 40-hex 'patch id', which can > -be obtained by passing the patch through the `git patch-id --unstable` > +be obtained by passing the patch through the `git patch-id --stable` > command. > > Imagine that on top of the public commit P, you applied well-known > diff --git a/builtin/log.c b/builtin/log.c > index e43ee12fb1dd..147850dc7317 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1435,7 +1435,7 @@ static void prepare_bases(struct base_tree_info *bases, > struct object_id *patch_id; > if (*commit_base_at(&commit_base, commit)) > continue; > - if (commit_patch_id(commit, &diffopt, &oid, 0)) > + if (commit_patch_id(commit, &diffopt, &oid, 0, 1)) > die(_("cannot get patch id")); > ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id); > patch_id = bases->patch_id + bases->nr_patch_id; > diff --git a/builtin/patch-id.c b/builtin/patch-id.c > index 970d0d30b4f4..bd28b80b2d0f 100644 > --- a/builtin/patch-id.c > +++ b/builtin/patch-id.c > @@ -1,5 +1,6 @@ > #include "builtin.h" > #include "config.h" > +#include "diff.h" > > static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result) > { > @@ -54,22 +55,6 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after) > return 1; > } > > -static void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx) > -{ > - unsigned char hash[GIT_MAX_RAWSZ]; > - unsigned short carry = 0; > - int i; > - > - git_SHA1_Final(hash, ctx); > - git_SHA1_Init(ctx); > - /* 20-byte sum, with carry */ > - for (i = 0; i < GIT_SHA1_RAWSZ; ++i) { > - carry += result->hash[i] + hash[i]; > - result->hash[i] = carry; > - carry >>= 8; > - } > -} > - > static int get_one_patchid(struct object_id *next_oid, struct object_id *result, > struct strbuf *line_buf, int stable) > { > diff --git a/diff.c b/diff.c > index 4d3cf83a27e5..8e3675b416a2 100644 > --- a/diff.c > +++ b/diff.c > @@ -5988,6 +5988,22 @@ static int remove_space(char *line, int len) > return dst - line; > } > > +void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx) > +{ > + unsigned char hash[GIT_MAX_RAWSZ]; > + unsigned short carry = 0; > + int i; > + > + git_SHA1_Final(hash, ctx); > + git_SHA1_Init(ctx); > + /* 20-byte sum, with carry */ > + for (i = 0; i < GIT_SHA1_RAWSZ; ++i) { > + carry += result->hash[i] + hash[i]; > + result->hash[i] = carry; > + carry >>= 8; > + } > +} > + > static void patch_id_consume(void *priv, char *line, unsigned long len) > { > struct patch_id_t *data = priv; > @@ -6012,8 +6028,8 @@ static void patch_id_add_mode(git_SHA_CTX *ctx, unsigned mode) > git_SHA1_Update(ctx, buf, len); > } > > -/* returns 0 upon success, and writes result into sha1 */ > -static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only) > +/* returns 0 upon success, and writes result into oid */ > +static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only, int stable) > { > struct diff_queue_struct *q = &diff_queued_diff; > int i; > @@ -6023,6 +6039,7 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid > git_SHA1_Init(&ctx); > memset(&data, 0, sizeof(struct patch_id_t)); > data.ctx = &ctx; > + oidclr(oid); > > for (i = 0; i < q->nr; i++) { > xpparam_t xpp; > @@ -6098,17 +6115,22 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid > patch_id_consume, &data, &xpp, &xecfg)) > return error("unable to generate patch-id diff for %s", > p->one->path); > + > + if (stable) > + flush_one_hunk(oid, &ctx); > } > > - git_SHA1_Final(oid->hash, &ctx); > + if (!stable) > + git_SHA1_Final(oid->hash, &ctx); > + > return 0; > } > > -int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only) > +int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only, int stable) > { > struct diff_queue_struct *q = &diff_queued_diff; > int i; > - int result = diff_get_patch_id(options, oid, diff_header_only); > + int result = diff_get_patch_id(options, oid, diff_header_only, stable); > > for (i = 0; i < q->nr; i++) > diff_free_filepair(q->queue[i]); > diff --git a/diff.h b/diff.h > index b20cbcc09142..de24a048649e 100644 > --- a/diff.h > +++ b/diff.h > @@ -436,7 +436,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option); > int run_diff_index(struct rev_info *revs, int cached); > > int do_diff_cache(const struct object_id *, struct diff_options *); > -int diff_flush_patch_id(struct diff_options *, struct object_id *, int); > +int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int); > +void flush_one_hunk(struct object_id *, git_SHA_CTX *); > > int diff_result_code(struct diff_options *, int); > > diff --git a/patch-ids.c b/patch-ids.c > index c262e1be9c9c..f70d3966542d 100644 > --- a/patch-ids.c > +++ b/patch-ids.c > @@ -11,7 +11,7 @@ static int patch_id_defined(struct commit *commit) > } > > int commit_patch_id(struct commit *commit, struct diff_options *options, > - struct object_id *oid, int diff_header_only) > + struct object_id *oid, int diff_header_only, int stable) > { > if (!patch_id_defined(commit)) > return -1; > @@ -22,7 +22,7 @@ int commit_patch_id(struct commit *commit, struct diff_options *options, > else > diff_root_tree_oid(&commit->object.oid, "", options); > diffcore_std(options); > - return diff_flush_patch_id(options, oid, diff_header_only); > + return diff_flush_patch_id(options, oid, diff_header_only, stable); > } > > /* > @@ -46,11 +46,11 @@ static int patch_id_neq(const void *cmpfn_data, > struct patch_id *b = (void *)entry_or_key; > > if (is_null_oid(&a->patch_id) && > - commit_patch_id(a->commit, opt, &a->patch_id, 0)) > + commit_patch_id(a->commit, opt, &a->patch_id, 0, 0)) > return error("Could not get patch ID for %s", > oid_to_hex(&a->commit->object.oid)); > if (is_null_oid(&b->patch_id) && > - commit_patch_id(b->commit, opt, &b->patch_id, 0)) > + commit_patch_id(b->commit, opt, &b->patch_id, 0, 0)) > return error("Could not get patch ID for %s", > oid_to_hex(&b->commit->object.oid)); > return !oideq(&a->patch_id, &b->patch_id); > @@ -80,7 +80,7 @@ static int init_patch_id_entry(struct patch_id *patch, > struct object_id header_only_patch_id; > > patch->commit = commit; > - if (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1)) > + if (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1, 0)) > return -1; > > hashmap_entry_init(patch, sha1hash(header_only_patch_id.hash)); > diff --git a/patch-ids.h b/patch-ids.h > index 82a12b66f889..03bb04e7071f 100644 > --- a/patch-ids.h > +++ b/patch-ids.h > @@ -20,7 +20,7 @@ struct patch_ids { > }; > > int commit_patch_id(struct commit *commit, struct diff_options *options, > - struct object_id *oid, int); > + struct object_id *oid, int, int); > int init_patch_ids(struct repository *, struct patch_ids *); > int free_patch_ids(struct patch_ids *); > struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *); > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > index e82c6c7d9177..7a5833a4b275 100755 > --- a/t/t4014-format-patch.sh > +++ b/t/t4014-format-patch.sh > @@ -1583,15 +1583,15 @@ test_expect_success 'format-patch --base' ' > git format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 && > echo >expected && > echo "base-commit: $(git rev-parse HEAD~3)" >>expected && > - echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk "{print \$1}")" >>expected && > - echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk "{print \$1}")" >>expected && > + echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>expected && > + echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>expected && > signature >> expected && > test_cmp expected actual1 && > test_cmp expected actual2 && > echo >fail && > echo "base-commit: $(git rev-parse HEAD~3)" >>fail && > - echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>fail && > - echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>fail && > + echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk "{print \$1}")" >>fail && > + echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk "{print \$1}")" >>fail && > signature >> fail && > ! test_cmp fail actual1 && > ! test_cmp fail actual2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] format-patch: Make --base patch-id output stable 2019-05-07 4:38 ` Junio C Hamano @ 2019-05-07 17:46 ` Stephen Boyd 2019-05-08 2:56 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Stephen Boyd @ 2019-05-07 17:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Xiaolong Ye Quoting Junio C Hamano (2019-05-06 21:38:24) > Stephen Boyd <sboyd@kernel.org> writes: > > > I wonder if we need to make some other sort of form of > > "prerequisite-patch-id:" here and let that be a legacy form of the > > patch-id so that users know that they have a fixed version of this code? > > Maybe "prerequisite-stable-patch-id:"? Or we don't have to care because > > it's been broken for anything besides the most trivial type of patches > > and presumably users aren't able to use it with 'patch-id --stable'? > > Do projects actively use -O<orderfile> when generating the patches? > I had an impression that not many do, and without -O<orderfile> in > the picture, --unstable/--stable would not matter, no? > > So, I am not sure if this matters very much in practice. > I'm not really concerned with projects using -O<orderfile> for patch generation. I'm concerned that the documentation told users to use --stable and that didn't work for me when there was more than one hunk in the patch. This leads me to believe that either I'm doing something wrong or the other users of this feature have been using --unstable since this was added to git format-patch. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] format-patch: Make --base patch-id output stable 2019-05-07 17:46 ` Stephen Boyd @ 2019-05-08 2:56 ` Junio C Hamano 0 siblings, 0 replies; 5+ messages in thread From: Junio C Hamano @ 2019-05-08 2:56 UTC (permalink / raw) To: Stephen Boyd; +Cc: git, Xiaolong Ye Stephen Boyd <sboyd@kernel.org> writes: > Quoting Junio C Hamano (2019-05-06 21:38:24) >> Stephen Boyd <sboyd@kernel.org> writes: >> >> > I wonder if we need to make some other sort of form of >> > "prerequisite-patch-id:" here and let that be a legacy form of the >> > patch-id so that users know that they have a fixed version of this code? >> > Maybe "prerequisite-stable-patch-id:"? Or we don't have to care because >> > it's been broken for anything besides the most trivial type of patches >> > and presumably users aren't able to use it with 'patch-id --stable'? >> >> Do projects actively use -O<orderfile> when generating the patches? >> I had an impression that not many do, and without -O<orderfile> in >> the picture, --unstable/--stable would not matter, no? >> >> So, I am not sure if this matters very much in practice. >> > > I'm not really concerned with projects using -O<orderfile> for patch > generation. I think I misunderstood, then. I have been assuming that the order of target file paths was the primary thing that contributes to the differences between --[un]stable modes, but apparently I forgot about that 30e12b92 ("patch-id: make it stable against hunk reordering", 2014-04-27) affects even a patch that touches a single path. If we advise "--stable" in the documentation to those who wants to interpret "--base", then I agree with the goal of this series to make sure that is what actually is happening. Thanks for working on this. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-08 2:56 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-26 23:51 [PATCH 1/2] format-patch: Inform user that patch-id generation is unstable Stephen Boyd 2019-04-26 23:51 ` [PATCH 2/2] format-patch: Make --base patch-id output stable Stephen Boyd 2019-05-07 4:38 ` Junio C Hamano 2019-05-07 17:46 ` Stephen Boyd 2019-05-08 2:56 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git 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 read-only IMAP folder(s) and NNTP newsgroup(s).