Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 30, 2018, 20:26 UTC
- Message-ID
- <CAPig+cQEFQah4VM3keTE94Yj9aXp1oKhgLwsz6=m8jJMTTmecQ@mail.gmail.com>
- In-Reply-To
- <CAGZ79kZSjwsk-YE1G3sGX-sfdu17NjMo-Jbrw2E5AbpbNGSGyg@mail.gmail.com>
On Wed, May 30, 2018 at 2:58 PM, Stefan Beller <sbeller@google.com> wrote:
Show 38 quoted lines
> On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> When submitting a revised a patch series, the --range-diff option embeds
>> a range-diff in the cover letter showing changes since the previous
>> version of the patch series. The argument to --range-diff is a simple
>> revision naming the tip of the previous series, which works fine if the
>> previous and current versions of the patch series share a common base.
>>
>> However, it fails if the revision ranges of the old and new versions of
>> the series are disjoint. To address this shortcoming, extend
>> --range-diff to also accept an explicit revision range for the previous
>> series. For example:
>>
>> git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>> static void infer_diff_ranges(struct argv_array *args,
>> const char *prev,
>> + struct commit *origin,
>> struct commit *head)
>> {
>> - argv_array_pushf(args, "%s...%s", prev,
>> - oid_to_hex(&head->object.oid));
>> + if (strstr(prev, "..")) {
>> + if (!origin)
>> + die(_("failed to infer range-diff ranges"));
>
>> static int get_range_diff(struct strbuf *sb,
>> @@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>> /* might die from bad user input so try before creating cover letter */
>> if (range_diff) {
>> struct argv_array ranges = ARGV_ARRAY_INIT;
>> - infer_diff_ranges(&ranges, range_diff, head);
>> + infer_diff_ranges(&ranges, range_diff, origin, head);
>
> This is way before the check for 'if (origin) emit_diffstat' as done in
> patch 1, as we want to do this early. We need to check the non-NULLness
> in infer_diff_ranges [...]Correct.
Show 9 quoted lines
> Would it make sense to give a better error message in:
>
>> + if (!origin)
>> + die(_("failed to infer range-diff ranges"));
>
> above? (The failure sounds like it could be anything, but the
> reason for it is actually well understood: The origin was
> computed and as the boundary commit of the given range,
> or NULL if there is no boundary commit or multiple commits.I'm not entirely happy with the generic error message either but didn't come up with anything better which was both succinct and actually helpful. I was hoping that a reviewer might suggest something better.
> If we find not exactly one boundary, we cannot compute the > range to give to the range-diff tool.
I considered allowing the argument to --range-diff to be much more complex: to provide a way for the user to supply all information needed for the invocation of git-range-diff (basically, to manually supply arguments to git-range-diff) for cases when inference wasn't possible. I even went so far as to consider allowing the 'creation-weight' to be specified as part of the --range-diff argument. However, that sort of complexity is both difficult to explain (document) and tends to be painful for users to specify (type correctly).
Therefore, in the end, I opted for simplicity: namely, handle the common cases with straight-forward minimal-learning-curve standard options. Both --range-diff=v3 and --range-diff=v3~4..v3 are easy to document and understand, as is the distinct --creation-weight= option. This seems a good balance between extreme flexibility and relative simplicity for handling common needs. (And, --range-diff is just a convenience, after all; more complex scenarios can still be handled manually by invoking git-range-diff followed by copy/paste.)
Show 23 quoted lines
> Stepping back to the error message, I have no good
> suggestion on what to say there. Maybe we'd want to
> refactor the code in cmd_format_patch, that computes the
> origin:
>
> if (prepare_revision_walk(&rev))
> die(_("revision walk setup failed"));
> rev.boundary = 1;
> while ((commit = get_revision(&rev)) != NULL) {
> if (commit->object.flags & BOUNDARY) {
> boundary_count++;
> origin = (boundary_count == 1) ? commit : NULL;
> continue;
> }
>
> We could prefix that with
>
> need_origin = (range_diff && strstr(prev, "..") || emit_diff_stat;
>
> and then if need_origin is about to be unset again we could issue
> a warning and die early after the loop warning about bad DAG form?
>
> I guess that can wait for a follow up patch.I agree. The feature can be improved and made more fancy incrementally as we gain better understanding of areas which need improvement. As a first step, I wanted to keep it minimal but usable for the most common cases.
Thanks for the review.