Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range

2 messages, 2 authors, 2021-01-25 · open the first message on its own page

Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range

From: Junio C Hamano <hidden>
Date: 2021-01-22 20:49:59

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:
From: Johannes Schindelin <redacted>

Currently, when called with exactly two arguments, `git range-diff`
tests for a literal `..` in each of the two. Likewise, the argument
provided via `--range-diff` to `git format-patch` is checked in the same
manner.

However, `<commit>^!` is a perfectly valid commit range, equivalent to
`<commit>^..<commit>` according to the `SPECIFYING RANGES` section of
gitrevisions[7].

In preparation for allowing more sophisticated ways to specify commit
ranges, let's refactor the check into its own function.
I think the sharing between the two makes sense, but the helper
function should make it clear in its name that this is "the kind of
commit range range-diff wants to take".  Among the commit range "git
log" and friends can take, range-diff can take only a subset of it,
and only a subset of it is meaningful to range-diff (e.g. HEAD^@ is
still a commit range you can give to "git log", but it would not
make much sense to give it to range-diff).

Perhaps

    s/specifies_commit_range/is_range_diff_range/

or something.
quoted hunk
diff --git a/revision.h b/revision.h
Move this to range-diff.h, not revision.h which is about true commit
ranges.
quoted hunk
index 086ff10280d..66777c8e60f 100644
--- a/revision.h
+++ b/revision.h
@@ -457,4 +457,11 @@ int rewrite_parents(struct rev_info *revs,
  */
 struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit);
 
+/*
+ * Determine whether the given argument defines a commit range, e.g. A..B.
+ * Note that this only validates the format but does _not_ parse it, i.e.
+ * it does _not_ look up the specified commits in the local repository.
+ */
And s/defines a commit range/is usable as a range to range-diff/

Thanks.
+int specifies_commit_range(const char *range);
+
 #endif

Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range

From: Uwe Kleine-König <hidden>
Date: 2021-01-25 07:37:06

On Fri, Jan 22, 2021 at 12:27:10PM -0800, Junio C Hamano wrote:
"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:
quoted
From: Johannes Schindelin <redacted>

Currently, when called with exactly two arguments, `git range-diff`
tests for a literal `..` in each of the two. Likewise, the argument
provided via `--range-diff` to `git format-patch` is checked in the same
manner.

However, `<commit>^!` is a perfectly valid commit range, equivalent to
`<commit>^..<commit>` according to the `SPECIFYING RANGES` section of
gitrevisions[7].

In preparation for allowing more sophisticated ways to specify commit
ranges, let's refactor the check into its own function.
I think the sharing between the two makes sense, but the helper
function should make it clear in its name that this is "the kind of
commit range range-diff wants to take".  Among the commit range "git
log" and friends can take, range-diff can take only a subset of it,
and only a subset of it is meaningful to range-diff (e.g. HEAD^@ is
still a commit range you can give to "git log", but it would not
make much sense to give it to range-diff).
Does it make so little sense to forbid passing HEAD^@ as a range to
range-diff? I can imagine situations where is would make sense, e.g. I
often create customer patch stacks from a set of topic branches using
octopus merge. To compare two of these ^@ might be handy.

My POV is that if it's easy to use the same function (and so the same
set of range descriptors) for git log and git range-diff then do so.
This yields a consistent behaviour which is IMHO better than preventing
people to do things that are considered strange today.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help