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

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

[Ramkumar Ramachandra]

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

[PATCH 1/2] git-format-patch: Print a diagnostic message when ignoring commits

[Ramkumar Ramachandra]

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 <jnareb@gmail.com>
Cc: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 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

[PATCH 2/2] log: Improve description of '-<n>' option in documentation

[Ramkumar Ramachandra]

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 <artagnon@gmail.com>
---
 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

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

[Ramkumar Ramachandra]

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

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Ramkumar Ramachandra writes:

>> 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

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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?

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?).  

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

[Ramkumar Ramachandra]

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:
> > Ramkumar Ramachandra writes:
> 
> >> 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.

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

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

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

> Jonathan Nieder writes:
>
>> 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.

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

[Thomas Rast]

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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ramkumar Ramachandra wrote:
> ...
>> 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.

Wait a bit.  How would you represent a merge in a patch form that can be
read by "git apply" (and "patch") in the first place?

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

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Ramkumar Ramachandra wrote:
> > ...
> >> 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.
> 
> Wait a bit.  How would you represent a merge in a patch form that can be
> read by "git apply" (and "patch") in the first place?

Oh, I'm not attempting that. I merely wanted people to be able to turn
off `--no-merges` so they can get the current functionality.

As far as represeting merge commits go, I'm still thinking about it. I
talked to Thomas about it briefly on IRC. The main issues:
1. What's the point? When there's no conflict resolution, a merge
   commit would be empty.
2. How do we uniquely specify what to merge? We can't use branch names
   or commit SHA1s, because they can change.

-- Ram