[PATCH 0/2] log/ format-patch improvements

Hi,

The first patch implements Jakub's suggestion. Arguably, it's slightly
complicated- it took me more than a few minutes to do the math with
`nr` and `nr_i`.

The second patch clarifies the meaning of the `-<n>` option. We should
also probably force the mutual exclusivity of `-<n>` and <revision
range> to avoid confusion.

Additionally, thanks to Thomas for drilling into me the fundamental
difference between -<n> and a revision range (on IRC).

Ramkumar Ramachandra (2):
  git-format-patch: Print a diagnostic message when ignoring commits
  log: Improve description of '-<n>' option in documentation

 Documentation/git-format-patch.txt |    2 +-
 Documentation/git-log.txt          |    2 +-
 builtin/log.c                      |   42 ++++++++++++++++++++++++++---------
 3 files changed, 33 insertions(+), 13 deletions(-)

-- 
1.7.2.2.409.gdbb11.dirty

The earlier description of the '-<n>' option was misleading- the user
would have expected to be able to use it to limit the number of
commits shown when specifying a revision range, for example. In
reality, the option simply instructs the log to walk the topmost <n>
commits. Also update the meaning of the same option in the
git-format-patch documentation.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 Documentation/git-format-patch.txt |    2 +-
 Documentation/git-log.txt          |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 4b3f5ba..df77474 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt

@@ -74,7 +74,7 @@ OPTIONS
 include::diff-options.txt[]
 
 -<n>::
-	Limits the number of patches to prepare.
+	Prepare patches from the topmost <n> commits.
 
 -o <dir>::
 --output-directory <dir>::

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 83e4ee3..ca02206 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt

@@ -28,7 +28,7 @@ OPTIONS
 -------
 
 -<n>::
-	Limits the number of commits to show.
+	Show the topmost <n> commits.
 
 <since>..<until>::
 	Show only commits between the named two commits.  When

-- 
1.7.2.2.409.gdbb11.dirty

Earlier, git-format-patch used to silently skip over commits that it
didn't intend to make patches out of. As a consequence, a command like
'git-format-patch -3' would just do nothing and print nothing if the
topmost three commits were merge commits. Instead, print a useful
message similar to "Skipping: Merge branch ..." when ignoring a
commit.

Suggested-by: Jakub Narebski <redacted>
Cc: Thomas Rast <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
 builtin/log.c |   42 +++++++++++++++++++++++++++++++-----------
 1 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 0151d2f..b64de7c 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -1090,7 +1090,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct commit **list = NULL;
 	struct rev_info rev;
 	struct setup_revision_opt s_r_opt;
-	int nr = 0, total, i;
+	int nr = 0, nr_i = 0, total, i;
 	int use_stdout = 0;
 	int start_number = -1;
 	int numbered_files = 0;		/* _just_ numbers */

@@ -1098,6 +1098,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int cover_letter = 0;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
+	int *list_i = NULL;
 	struct commit *origin = NULL, *head = NULL;
 	const char *in_reply_to = NULL;
 	struct patch_ids ids;

@@ -1342,19 +1343,22 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		/* ignore merges */
-		if (commit->parents && commit->parents->next)
-			continue;
-
-		if (ignore_if_in_upstream &&
-				has_commit_patch_id(commit, &ids))
-			continue;
+		/* ignore merge commits and optionally ignore commits
+		   already in upstream */
+		if ((commit->parents && commit->parents->next) ||
+		    (ignore_if_in_upstream &&
+		     has_commit_patch_id(commit, &ids))) {
+			/* Store the nr of the ignored commits in list_i */
+			nr_i++;
+			list_i = xrealloc(list_i, nr_i * sizeof(list_i[0]));
+			list_i[nr_i - 1] = nr;
+		}
 
 		nr++;
 		list = xrealloc(list, nr * sizeof(list[0]));
 		list[nr - 1] = commit;
 	}
-	total = nr;
+	total = nr - nr_i;
 	if (!keep_subject && auto_number && total > 1)
 		numbered = 1;
 	if (numbered)

@@ -1376,10 +1380,25 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		start_number--;
 	}
 	rev.add_signoff = add_signoff;
-	while (0 <= --nr) {
+	for (i = nr - nr_i; --nr >= 0;) {
 		int shown;
 		commit = list[nr];
-		rev.nr = total - nr + (start_number - 1);
+
+		/* Ignore commits in list whose index is list_i */
+		if (list_i[nr_i - 1] == nr) {
+			struct strbuf commit_msg = STRBUF_INIT;
+			struct pretty_print_context ctx = {0};
+			format_commit_message(commit, "%s", &commit_msg, &ctx);
+			fprintf(realstdout, "Skipping: %s\n",
+				commit_msg.buf);
+			strbuf_release(&buf);
+			--nr_i;
+			continue;
+		}
+		else
+			--i;
+
+		rev.nr = total - i + (start_number - 1);
 		/* Make the second and subsequent mails replies to the first */
 		if (thread) {
 			/* Have we already had a message ID? */

@@ -1443,6 +1462,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			fclose(stdout);
 	}
 	free(list);
+	free(list_i);
 	string_list_clear(&extra_to, 0);
 	string_list_clear(&extra_cc, 0);
 	string_list_clear(&extra_hdr, 0);

-- 
1.7.2.2.409.gdbb11.dirty

Hi,

Ramkumar Ramachandra writes:

The first patch implements Jakub's suggestion. Arguably, it's slightly
complicated- it took me more than a few minutes to do the math with
`nr` and `nr_i`.

The second patch clarifies the meaning of the `-<n>` option. We should
also probably force the mutual exclusivity of `-<n>` and <revision
range> to avoid confusion.

Additionally, thanks to Thomas for drilling into me the fundamental
difference between -<n> and a revision range (on IRC).

Ramkumar Ramachandra (2):
  git-format-patch: Print a diagnostic message when ignoring commits
  log: Improve description of '-<n>' option in documentation

 Documentation/git-format-patch.txt |    2 +-
 Documentation/git-log.txt          |    2 +-
 builtin/log.c                      |   42 ++++++++++++++++++++++++++---------
 3 files changed, 33 insertions(+), 13 deletions(-)

Do you see value in this patch or is it just unnecessary baggage?

-- Ram

Ramkumar Ramachandra wrote:

Ramkumar Ramachandra writes:

quoted

The second patch clarifies the meaning of the `-<n>` option. We should
also probably force the mutual exclusivity of `-<n>` and <revision
range> to avoid confusion.

[...]

Do you see value in this patch or is it just unnecessary baggage?

I see value in avoiding confusion.  Maybe one solution would be to make
format-patch use --no-merges by default.

 $ git log --oneline --no-merges -3 ab/test..origin/pu
 70256a3 shell: Rewrite documentation and improve error message
 9c46c05 rev-parse: tests git rev-parse --verify master@{n}, for various n
 eedce78 sha1_name.c: use warning in preference to fprintf(stderr

Hi Jonathan and Junio,

Junio C Hamano writes:

I am not very impressed by the counting.  It probably makes more sense to
count only what we are actually going to process and emit, i.e. always use
no-merges (do we even support format-patch on a merge?).  

Frankly, I think the patch looks like an ugly hack myself. No,
format-patch doesn't support merge commits at all.

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

Ramkumar Ramachandra writes:

quoted

quoted

The second patch clarifies the meaning of the `-<n>` option. We should
also probably force the mutual exclusivity of `-<n>` and <revision
range> to avoid confusion.

[...]

quoted

Do you see value in this patch or is it just unnecessary baggage?

I see value in avoiding confusion.  Maybe one solution would be to make
format-patch use --no-merges by default.

Good idea. I'll write a patch. Do we also want people to be able to
turn off `--no-merges`? If so, how?

-- Ram

Ramkumar Ramachandra wrote:

No,
format-patch doesn't support merge commits at all.

[...]

Jonathan Nieder writes:

quoted

I see value in avoiding confusion.  Maybe one solution would be to make
format-patch use --no-merges by default.

Good idea. I'll write a patch. Do we also want people to be able to
turn off `--no-merges`?

I don't see a need for it.

However, if you can think of good names for --undo-no-merges and
--undo-merges options to "git log", that might be a nice independent
change for the revision option parser.

Jonathan Nieder wrote:

However, if you can think of good names for --undo-no-merges and
--undo-merges options to "git log", that might be a nice independent
change for the revision option parser.

--merges={include,only,exclude} or so, and then have --merges be
--merges=only and --no-merges be --merges=exclude?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch