Diffing submodule does not yield complete logs for merge commits

17 messages, 6 authors, 2016-06-15 · open the first message on its own page

Diffing submodule does not yield complete logs for merge commits

From: Robert Dailey <hidden>
Date: 2016-06-15 23:04:34

I am attempting to diff a submodule modified in my working copy and
the only difference is a merge commit. However, I do not get the
"full" range of commits introduced by the merge commit when I diff it:

$ git diff --submodule=log Core
Submodule Core 8b4ec60..def2f3b:
  > Merge remote-tracking branch 'origin/master-ah3k'

However if I go inside my submodule and run `git log` by hand, I get
more information about the TRUE commits introduced:

$ git log --oneline 8b4ec60..def2f3b
def2f3b Merge remote-tracking branch 'origin/master-ah3k'
015c961 Remove log spam in FontManager
7713ba1 Update third party submodule to latest
10aac78 Merge pull request #9 in FE/core from
feature/FE-1348-selecting-continue-on-zero-balance to master-ah3k
287882f FE-1376 Nedd to remain in check detail screen when selecting
donation after SBI
a5a6bed Do not overwrite the current check# within loop
dfb8547 Adding list of checks to CRspChecks before saving
1be280a FE-1354: Guest logged out in specific multiple check scenario
de06d5a [FE-1348] Fix PATT exit while checks still open

It's almost as if the `git diff --submodule=log` approach is passing
in --first-parent to git log, which would exclude commits in the range
that I'm seeing when I run git log manually.

Is this by design? Is there a way to enable the full log history with
`git diff` on a submodule?

Re: Diffing submodule does not yield complete logs for merge commits

From: Heiko Voigt <hidden>
Date: 2016-06-15 23:04:36

Hi,

On Wed, Apr 29, 2015 at 03:53:11PM -0500, Robert Dailey wrote:
I am attempting to diff a submodule modified in my working copy and
the only difference is a merge commit. However, I do not get the
"full" range of commits introduced by the merge commit when I diff it:

$ git diff --submodule=log Core
Submodule Core 8b4ec60..def2f3b:
  > Merge remote-tracking branch 'origin/master-ah3k'

However if I go inside my submodule and run `git log` by hand, I get
more information about the TRUE commits introduced:

$ git log --oneline 8b4ec60..def2f3b
def2f3b Merge remote-tracking branch 'origin/master-ah3k'
015c961 Remove log spam in FontManager
7713ba1 Update third party submodule to latest
10aac78 Merge pull request #9 in FE/core from
feature/FE-1348-selecting-continue-on-zero-balance to master-ah3k
287882f FE-1376 Nedd to remain in check detail screen when selecting
donation after SBI
a5a6bed Do not overwrite the current check# within loop
dfb8547 Adding list of checks to CRspChecks before saving
1be280a FE-1354: Guest logged out in specific multiple check scenario
de06d5a [FE-1348] Fix PATT exit while checks still open

It's almost as if the `git diff --submodule=log` approach is passing
in --first-parent to git log, which would exclude commits in the range
that I'm seeing when I run git log manually.
That is exactly the case. In prepare_submodule_summary() that option is
set before doing the revision walk.
Is this by design? Is there a way to enable the full log history with
`git diff` on a submodule?
This stems from the first implementation for showing submodule diffs in
commit 752c0c24. I guess this was done deliberately to limit the amount
of output you get for a submodule. At the moment this is hardcoded but I
think there is nothing wrong with adding another option to include the
full log.

Cheers Heiko

Re: Diffing submodule does not yield complete logs for merge commits

From: Robert Dailey <hidden>
Date: 2016-06-15 23:04:38

On Fri, May 1, 2015 at 12:57 PM, Heiko Voigt [off-list ref] wrote:
Hi,

On Wed, Apr 29, 2015 at 03:53:11PM -0500, Robert Dailey wrote:
quoted
I am attempting to diff a submodule modified in my working copy and
the only difference is a merge commit. However, I do not get the
"full" range of commits introduced by the merge commit when I diff it:

$ git diff --submodule=log Core
Submodule Core 8b4ec60..def2f3b:
  > Merge remote-tracking branch 'origin/master-ah3k'

However if I go inside my submodule and run `git log` by hand, I get
more information about the TRUE commits introduced:

$ git log --oneline 8b4ec60..def2f3b
def2f3b Merge remote-tracking branch 'origin/master-ah3k'
015c961 Remove log spam in FontManager
7713ba1 Update third party submodule to latest
10aac78 Merge pull request #9 in FE/core from
feature/FE-1348-selecting-continue-on-zero-balance to master-ah3k
287882f FE-1376 Nedd to remain in check detail screen when selecting
donation after SBI
a5a6bed Do not overwrite the current check# within loop
dfb8547 Adding list of checks to CRspChecks before saving
1be280a FE-1354: Guest logged out in specific multiple check scenario
de06d5a [FE-1348] Fix PATT exit while checks still open

It's almost as if the `git diff --submodule=log` approach is passing
in --first-parent to git log, which would exclude commits in the range
that I'm seeing when I run git log manually.
That is exactly the case. In prepare_submodule_summary() that option is
set before doing the revision walk.
quoted
Is this by design? Is there a way to enable the full log history with
`git diff` on a submodule?
This stems from the first implementation for showing submodule diffs in
commit 752c0c24. I guess this was done deliberately to limit the amount
of output you get for a submodule. At the moment this is hardcoded but I
think there is nothing wrong with adding another option to include the
full log.

Cheers Heiko
I will go ahead and work on this feature. Here is what I'd like to see:

1. `git diff --submodule` should have the ability to display full logs
vs current logs (i.e. without --first-parent)
2. `git submodule summary` should have an option to display full logs
or "first-parent" logs.

For #1, do you recommend adding a 3rd setting for `diff.submodule`
config? Something like "full-log" or something? Or an entirely new
config? I noticed that in diff.h, the DIFF_OPT flags already consume
31 bits. If this is a 32-bit flag, there is only 1 bit left. If we go
with a 3rd setting for `diff.submodule` I think this might consume the
last bit.

We could also make `git diff --submodule` default to the "full log"
type, and if users want only first parent logs in submodule summary,
they'd have to execute `git submodule summary` instead.

There are a few options. What do you recommend? Thanks.

Re: Diffing submodule does not yield complete logs for merge commits

From: Jens Lehmann <hidden>
Date: 2016-06-15 23:04:38

Am 04.05.2015 um 17:05 schrieb Robert Dailey:
On Fri, May 1, 2015 at 12:57 PM, Heiko Voigt [off-list ref] wrote:
quoted
Hi,

On Wed, Apr 29, 2015 at 03:53:11PM -0500, Robert Dailey wrote:
quoted
I am attempting to diff a submodule modified in my working copy and
the only difference is a merge commit. However, I do not get the
"full" range of commits introduced by the merge commit when I diff it:

$ git diff --submodule=log Core
Submodule Core 8b4ec60..def2f3b:
   > Merge remote-tracking branch 'origin/master-ah3k'

However if I go inside my submodule and run `git log` by hand, I get
more information about the TRUE commits introduced:

$ git log --oneline 8b4ec60..def2f3b
def2f3b Merge remote-tracking branch 'origin/master-ah3k'
015c961 Remove log spam in FontManager
7713ba1 Update third party submodule to latest
10aac78 Merge pull request #9 in FE/core from
feature/FE-1348-selecting-continue-on-zero-balance to master-ah3k
287882f FE-1376 Nedd to remain in check detail screen when selecting
donation after SBI
a5a6bed Do not overwrite the current check# within loop
dfb8547 Adding list of checks to CRspChecks before saving
1be280a FE-1354: Guest logged out in specific multiple check scenario
de06d5a [FE-1348] Fix PATT exit while checks still open

It's almost as if the `git diff --submodule=log` approach is passing
in --first-parent to git log, which would exclude commits in the range
that I'm seeing when I run git log manually.
That is exactly the case. In prepare_submodule_summary() that option is
set before doing the revision walk.
quoted
Is this by design? Is there a way to enable the full log history with
`git diff` on a submodule?
This stems from the first implementation for showing submodule diffs in
commit 752c0c24. I guess this was done deliberately to limit the amount
of output you get for a submodule. At the moment this is hardcoded but I
think there is nothing wrong with adding another option to include the
full log.

Cheers Heiko
I will go ahead and work on this feature. Here is what I'd like to see:

1. `git diff --submodule` should have the ability to display full logs
vs current logs (i.e. without --first-parent)
I agree. Just recently I started missing that feature too at $DAYJOB.
2. `git submodule summary` should have an option to display full logs
or "first-parent" logs.
No objection against that. Maybe now is a good time to make `git
submodule summary` use `git diff --submodule` internally to make
them behave the same?
For #1, do you recommend adding a 3rd setting for `diff.submodule`
config? Something like "full-log" or something? Or an entirely new
config?
I'd go with a 3rd setting for diff.submodule (and "full-log" would
have been my first choice too ;-).

 > I noticed that in diff.h, the DIFF_OPT flags already consume
31 bits. If this is a 32-bit flag, there is only 1 bit left. If we go
with a 3rd setting for `diff.submodule` I think this might consume the
last bit.
Yup. But I'm not sure we can do anything about it.
We could also make `git diff --submodule` default to the "full log"
type, and if users want only first parent logs in submodule summary,
they'd have to execute `git submodule summary` instead.
Please do not change defaults that people lived fine with for years
lightly. But I won't object changing that on a major version if a
majority of users request that.

Re: Diffing submodule does not yield complete logs for merge commits

From: Robert Dailey <hidden>
Date: 2016-06-15 23:04:38

On Mon, May 4, 2015 at 2:32 PM, Jens Lehmann [off-list ref] wrote:
Am 04.05.2015 um 17:05 schrieb Robert Dailey:
quoted
On Fri, May 1, 2015 at 12:57 PM, Heiko Voigt [off-list ref] wrote:
quoted
Hi,

On Wed, Apr 29, 2015 at 03:53:11PM -0500, Robert Dailey wrote:
quoted
I am attempting to diff a submodule modified in my working copy and
the only difference is a merge commit. However, I do not get the
"full" range of commits introduced by the merge commit when I diff it:

$ git diff --submodule=log Core
Submodule Core 8b4ec60..def2f3b:
   > Merge remote-tracking branch 'origin/master-ah3k'

However if I go inside my submodule and run `git log` by hand, I get
more information about the TRUE commits introduced:

$ git log --oneline 8b4ec60..def2f3b
def2f3b Merge remote-tracking branch 'origin/master-ah3k'
015c961 Remove log spam in FontManager
7713ba1 Update third party submodule to latest
10aac78 Merge pull request #9 in FE/core from
feature/FE-1348-selecting-continue-on-zero-balance to master-ah3k
287882f FE-1376 Nedd to remain in check detail screen when selecting
donation after SBI
a5a6bed Do not overwrite the current check# within loop
dfb8547 Adding list of checks to CRspChecks before saving
1be280a FE-1354: Guest logged out in specific multiple check scenario
de06d5a [FE-1348] Fix PATT exit while checks still open

It's almost as if the `git diff --submodule=log` approach is passing
in --first-parent to git log, which would exclude commits in the range
that I'm seeing when I run git log manually.

That is exactly the case. In prepare_submodule_summary() that option is
set before doing the revision walk.
quoted
Is this by design? Is there a way to enable the full log history with
`git diff` on a submodule?

This stems from the first implementation for showing submodule diffs in
commit 752c0c24. I guess this was done deliberately to limit the amount
of output you get for a submodule. At the moment this is hardcoded but I
think there is nothing wrong with adding another option to include the
full log.

Cheers Heiko

I will go ahead and work on this feature. Here is what I'd like to see:

1. `git diff --submodule` should have the ability to display full logs
vs current logs (i.e. without --first-parent)

I agree. Just recently I started missing that feature too at $DAYJOB.
quoted
2. `git submodule summary` should have an option to display full logs
or "first-parent" logs.

No objection against that. Maybe now is a good time to make `git
submodule summary` use `git diff --submodule` internally to make
them behave the same?
quoted
For #1, do you recommend adding a 3rd setting for `diff.submodule`
config? Something like "full-log" or something? Or an entirely new
config?

I'd go with a 3rd setting for diff.submodule (and "full-log" would
have been my first choice too ;-).
quoted
I noticed that in diff.h, the DIFF_OPT flags already consume

31 bits. If this is a 32-bit flag, there is only 1 bit left. If we go
with a 3rd setting for `diff.submodule` I think this might consume the
last bit.

Yup. But I'm not sure we can do anything about it.
quoted
We could also make `git diff --submodule` default to the "full log"
type, and if users want only first parent logs in submodule summary,
they'd have to execute `git submodule summary` instead.

Please do not change defaults that people lived fine with for years
lightly. But I won't object changing that on a major version if a
majority of users request that.
Since I am not a linux user, I have implemented this feature against
the Git for Windows fork of git. I am not able to verify changes if I
make them directly against the Git repository. Is it OK if you guys
end up getting this as an upstream patch later from that project? Also
I am not familiar with the bash unit tests, I will need help with
that.

Re: Diffing submodule does not yield complete logs for merge commits

From: Heiko Voigt <hidden>
Date: 2016-06-15 23:04:38

Hi,

On Mon, May 04, 2015 at 03:21:31PM -0500, Robert Dailey wrote:
Since I am not a linux user, I have implemented this feature against
the Git for Windows fork of git. I am not able to verify changes if I
make them directly against the Git repository. Is it OK if you guys
end up getting this as an upstream patch later from that project? Also
I am not familiar with the bash unit tests, I will need help with
that.
I think there is nothing wrong with implementing it in the Windows
development environment and then sending the patch directly here. As
long as it is not Windows specific (which it should not be) that should
be fine and you save the Windows guys the work to get the patch
upstream (because here is upstream not there ;-)).

Have a look at some tests, they are quite simple. Basically they run git
commands in a && chain and the resulting return code tells the testsuite
whether that test succeeded or not. Maybe have a look at the test for
the existing --submodule option (t4041-diff-submodule-option.sh) as an
example. You can probably reuse the complete setup there and just add a
new test for the new option with the expected output. There is also a
README in the t/ folder. HTH.

Cheers Heiko

Re: Diffing submodule does not yield complete logs for merge commits

From: Johannes Schindelin <hidden>
Date: 2016-06-15 23:04:39

Hi Robert,

On 2015-05-04 22:21, Robert Dailey wrote:
Since I am not a linux user, I have implemented this feature against
the Git for Windows fork of git. I am not able to verify changes if I
make them directly against the Git repository.
That is why I worked so hard on support of Vagrant: https://github.com/msysgit/msysgit/wiki/Vagrant -- in short, it makes it dead easy for you to set up a *minimal* Linux VM inside your Git SDK and interact with it via ssh.

With the Vagrant solution, you can easily test Linux Git even on Windows.

Ciao,
Johannes

Re: Diffing submodule does not yield complete logs for merge commits

From: Robert Dailey <hidden>
Date: 2016-06-15 23:04:47

On Tue, May 5, 2015 at 12:49 AM, Johannes Schindelin
[off-list ref] wrote:
Hi Robert,

On 2015-05-04 22:21, Robert Dailey wrote:
quoted
Since I am not a linux user, I have implemented this feature against
the Git for Windows fork of git. I am not able to verify changes if I
make them directly against the Git repository.
That is why I worked so hard on support of Vagrant: https://github.com/msysgit/msysgit/wiki/Vagrant -- in short, it makes it dead easy for you to set up a *minimal* Linux VM inside your Git SDK and interact with it via ssh.

With the Vagrant solution, you can easily test Linux Git even on Windows.

Ciao,
Johannes
At the moment I have a "half-ass" patch attached. This implements the
feature itself. I'm able to test this and it seems to be working.
Please note I'm a C++ developer and straight C / Bash are not my
strong suits. I apologize in advance for any mistakes. I am open to
taking recommendations for corrections.

I'm not sure how I can verify the feature in a unit test. In addition
to learning bash scripting well enough to write the test, I am not
sure how to use git to check for the additional commits. Plus the repo
for the test will need to handle a submodule change to a merge commit
as well. Any advice on setting up a good test case for this? What
conditions should I check for, as far as log output goes?

Re: Diffing submodule does not yield complete logs for merge commits

From: Heiko Voigt <hidden>
Date: 2016-06-15 23:04:48

Hi,

On Fri, May 15, 2015 at 03:33:07PM -0500, Robert Dailey wrote:
On Tue, May 5, 2015 at 12:49 AM, Johannes Schindelin
[off-list ref] wrote:
quoted
Hi Robert,

On 2015-05-04 22:21, Robert Dailey wrote:
quoted
Since I am not a linux user, I have implemented this feature against
the Git for Windows fork of git. I am not able to verify changes if I
make them directly against the Git repository.
That is why I worked so hard on support of Vagrant: https://github.com/msysgit/msysgit/wiki/Vagrant -- in short, it makes it dead easy for you to set up a *minimal* Linux VM inside your Git SDK and interact with it via ssh.

With the Vagrant solution, you can easily test Linux Git even on Windows.

Ciao,
Johannes
At the moment I have a "half-ass" patch attached. This implements the
feature itself. I'm able to test this and it seems to be working.
Please note I'm a C++ developer and straight C / Bash are not my
strong suits. I apologize in advance for any mistakes. I am open to
taking recommendations for corrections.
Please inline the patch, so people can easily comment. Have a look at
Documentation/SubmittingPatches and patches on this list for an example.
I have inlined your patch below for comments.
I'm not sure how I can verify the feature in a unit test. In addition
to learning bash scripting well enough to write the test, I am not
sure how to use git to check for the additional commits. Plus the repo
for the test will need to handle a submodule change to a merge commit
as well. Any advice on setting up a good test case for this? What
conditions should I check for, as far as log output goes?
The testsuite can be found in t/ the README there describes most of it.
Have a look at t4041-diff-submodule-option.sh and imitate the tests for
the existing log option. What they basically do is: Write a file with
the expected output of the diff and then compare the actual output with
it. That should also be possible for your option.

As for the merge commit: If there is no merge commit in the submodule
that is used for testing you can simply add a sequence of git commands
that manufactures the situation in the test repository as you need it.

'test_pause' is a helpful command to interactively debug/develop tests.
Run the test with the -v -i switches (maybe -d) when developing.

Comments for your patch please see below.

Cheers Heiko
quoted hunk
From: Robert Dailey <redacted>
Subject: [PATCH] Add 'full-log' option to diff.submodule

Like the 'log' option to `diff --submodule`, 'full-log' provides
logs without the `--first-parent` option.
---
 diff.c      | 16 ++++++++++++----
 diff.h      |  1 +
 submodule.c |  9 +++++----
 submodule.h |  3 ++-
 4 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/diff.c b/diff.c
index 7500c55..58c4872 100644
--- a/diff.c
+++ b/diff.c
@@ -128,10 +128,18 @@ static int parse_dirstat_params(struct diff_options *options, const char *params
 
 static int parse_submodule_params(struct diff_options *options, const char *value)
 {
-	if (!strcmp(value, "log"))
+	if (!strcmp(value, "log")) {
 		DIFF_OPT_SET(options, SUBMODULE_LOG);
-	else if (!strcmp(value, "short"))
+		DIFF_OPT_CLR(options, SUBMODULE_FULL_LOG);
+	}
+	else if (!strcmp(value, "full-log")) {
+		DIFF_OPT_SET(options, SUBMODULE_FULL_LOG);
+		DIFF_OPT_CLR(options, SUBMODULE_LOG);
+	}
+	else if (!strcmp(value, "short")) {
 		DIFF_OPT_CLR(options, SUBMODULE_LOG);
+		DIFF_OPT_CLR(options, SUBMODULE_FULL_LOG);
+	}
Here I think clearing the bits first and then setting them would be
simpler and less error prone for further extensions. E.g. in the
beginning of the function:

	DIFF_OPT_CLR(options, SUBMODULE_LOG);
	DIFF_OPT_CLR(options, SUBMODULE_FULL_LOG);

and then

	if (!strcmp(value, "log"))
		DIFF_OPT_SET(options, SUBMODULE_LOG);
	else if (...

quoted hunk
 	else
 		return -1;
 	return 0;
@@ -2240,7 +2248,7 @@ static void builtin_diff(const char *name_a,
 	struct strbuf header = STRBUF_INIT;
 	const char *line_prefix = diff_line_prefix(o);
 
-	if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
+	if ((DIFF_OPT_TST(o, SUBMODULE_LOG) || DIFF_OPT_TST(o, SUBMODULE_FULL_LOG)) &&
Try to keep your line length less than 80 characters.
(Documentation/CodingGuidelines)
quoted hunk
 			(!one->mode || S_ISGITLINK(one->mode)) &&
 			(!two->mode || S_ISGITLINK(two->mode))) {
 		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
@@ -2248,7 +2256,7 @@ static void builtin_diff(const char *name_a,
 		show_submodule_summary(o->file, one->path ? one->path : two->path,
 				line_prefix,
 				one->sha1, two->sha1, two->dirty_submodule,
-				meta, del, add, reset);
+				meta, del, add, reset, DIFF_OPT_TST(o, SUBMODULE_FULL_LOG));
Same as above.
quoted hunk
 		return;
 	}
 
diff --git a/diff.h b/diff.h
index b4a624d..95f319c 100644
--- a/diff.h
+++ b/diff.h
@@ -90,6 +90,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 28)
 #define DIFF_OPT_FUNCCONTEXT         (1 << 29)
 #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
+#define DIFF_OPT_SUBMODULE_FULL_LOG  (1 << 31)
 
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_TOUCHED(opts, flag)    ((opts)->touched_flags & DIFF_OPT_##flag)
diff --git a/submodule.c b/submodule.c
index d37d400..f98173e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -290,14 +290,14 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
 
 static int prepare_submodule_summary(struct rev_info *rev, const char *path,
 		struct commit *left, struct commit *right,
-		int *fast_forward, int *fast_backward)
+		int *fast_forward, int *fast_backward, unsigned full_log)
 {
 	struct commit_list *merge_bases, *list;
 
 	init_revisions(rev, NULL);
 	setup_revisions(0, NULL, rev, NULL);
 	rev->left_right = 1;
-	rev->first_parent_only = 1;
+	rev->first_parent_only = full_log ? 0 : 1;
 	left->object.flags |= SYMMETRIC_LEFT;
 	add_pending_object(rev, &left->object, path);
 	add_pending_object(rev, &right->object, path);
@@ -363,7 +363,8 @@ void show_submodule_summary(FILE *f, const char *path,
 		const char *line_prefix,
 		unsigned char one[20], unsigned char two[20],
 		unsigned dirty_submodule, const char *meta,
-		const char *del, const char *add, const char *reset)
+		const char *del, const char *add, const char *reset,
+		unsigned full_log)
 {
 	struct rev_info rev;
 	struct commit *left = NULL, *right = NULL;
@@ -381,7 +382,7 @@ void show_submodule_summary(FILE *f, const char *path,
 		 !(right = lookup_commit_reference(two)))
 		message = "(commits not present)";
 	else if (prepare_submodule_summary(&rev, path, left, right,
-					   &fast_forward, &fast_backward))
+					   &fast_forward, &fast_backward, full_log))
Line length.
quoted hunk
 		message = "(revision walker failed)";
 
 	if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
diff --git a/submodule.h b/submodule.h
index 7beec48..301358b 100644
--- a/submodule.h
+++ b/submodule.h
@@ -26,7 +26,8 @@ void show_submodule_summary(FILE *f, const char *path,
 		const char *line_prefix,
 		unsigned char one[20], unsigned char two[20],
 		unsigned dirty_submodule, const char *meta,
-		const char *del, const char *add, const char *reset);
+		const char *del, const char *add, const char *reset,
+		unsigned full_log);
 void set_config_fetch_recurse_submodules(int value);
 void check_for_new_submodule_commits(unsigned char new_sha1[20]);
 int fetch_populated_submodules(const struct argv_array *options,
Apart from the comments above, your patch looks good to me.

Cheers Heiko

Re: Diffing submodule does not yield complete logs for merge commits

From: Robert Dailey <hidden>
Date: 2016-06-15 23:04:48

On Mon, May 18, 2015 at 7:30 AM, Heiko Voigt [off-list ref] wrote:
Hi,

On Fri, May 15, 2015 at 03:33:07PM -0500, Robert Dailey wrote:
quoted
On Tue, May 5, 2015 at 12:49 AM, Johannes Schindelin
[off-list ref] wrote:
quoted
Hi Robert,

On 2015-05-04 22:21, Robert Dailey wrote:
quoted
Since I am not a linux user, I have implemented this feature against
the Git for Windows fork of git. I am not able to verify changes if I
make them directly against the Git repository.
That is why I worked so hard on support of Vagrant: https://github.com/msysgit/msysgit/wiki/Vagrant -- in short, it makes it dead easy for you to set up a *minimal* Linux VM inside your Git SDK and interact with it via ssh.

With the Vagrant solution, you can easily test Linux Git even on Windows.

Ciao,
Johannes
At the moment I have a "half-ass" patch attached. This implements the
feature itself. I'm able to test this and it seems to be working.
Please note I'm a C++ developer and straight C / Bash are not my
strong suits. I apologize in advance for any mistakes. I am open to
taking recommendations for corrections.
Please inline the patch, so people can easily comment. Have a look at
Documentation/SubmittingPatches and patches on this list for an example.
I have inlined your patch below for comments.
quoted
I'm not sure how I can verify the feature in a unit test. In addition
to learning bash scripting well enough to write the test, I am not
sure how to use git to check for the additional commits. Plus the repo
for the test will need to handle a submodule change to a merge commit
as well. Any advice on setting up a good test case for this? What
conditions should I check for, as far as log output goes?
The testsuite can be found in t/ the README there describes most of it.
Have a look at t4041-diff-submodule-option.sh and imitate the tests for
the existing log option. What they basically do is: Write a file with
the expected output of the diff and then compare the actual output with
it. That should also be possible for your option.

As for the merge commit: If there is no merge commit in the submodule
that is used for testing you can simply add a sequence of git commands
that manufactures the situation in the test repository as you need it.

'test_pause' is a helpful command to interactively debug/develop tests.
Run the test with the -v -i switches (maybe -d) when developing.

Comments for your patch please see below.

<snip>
Unfortunately I find it unintuitive and counter productive to perform
inline patches or do anything on a mailing list. Especially on
Windows, it's a pain to setup git to effectively do this. Also I read
mailing lists through Gmail which does not offer a proper monospace
font view or syntax coloring to effectively review patches and
comments pertaining to them.

Since I am not willing to properly follow your process, I will
withdraw my patch. However it is here if someone else wishes to take
it over. Really wish you guys used github's amazing features but I
understand that Linus has already made his decision in that matter.

I'm sorry I couldn't be more agreeable on the matter. Thanks for the
time you spent reviewing my patch.

Re: Diffing submodule does not yield complete logs for merge commits

From: Heiko Voigt <hidden>
Date: 2016-06-15 23:04:49

On Mon, May 18, 2015 at 10:06:32AM -0500, Robert Dailey wrote:
Unfortunately I find it unintuitive and counter productive to perform
inline patches or do anything on a mailing list. Especially on
Windows, it's a pain to setup git to effectively do this. Also I read
mailing lists through Gmail which does not offer a proper monospace
font view or syntax coloring to effectively review patches and
comments pertaining to them.
Are you sure you are not overestimating the effort it takes to send
patches inline? Once you've got your user agent correctly setup its just
a matter of copy and paste instead of attaching the patch. On Windows I
would probably use Thunderbird which has a section in the format-patch
documentation how to configure it. Compared to the effort you probably
spent on writing your patch isn't this bit of extra effort neglectable?
And your patch is almost done. It just needs some tests and maybe a few
rounds on the mailinglist after that.
Since I am not willing to properly follow your process, I will
withdraw my patch. However it is here if someone else wishes to take
it over. Really wish you guys used github's amazing features but I
understand that Linus has already made his decision in that matter.
It not just Linus decision it is also a matter of many people are used
to this workflow. AFAIR there have been many discussions and tries about
using other tools. Email has many advantages which a webinterface does
not provide. It is simply less effort that one person adjusts to this
workflow instead of changing many peoples working workflow.
I'm sorry I couldn't be more agreeable on the matter. Thanks for the
time you spent reviewing my patch.
If you are really this fixed in your workflow that would be too bad.

Cheers Heiko

Re: Diffing submodule does not yield complete logs for merge commits

From: Robert Dailey <hidden>
Date: 2016-06-15 23:04:49

On Tue, May 19, 2015 at 5:44 AM, Heiko Voigt [off-list ref] wrote:
On Mon, May 18, 2015 at 10:06:32AM -0500, Robert Dailey wrote:
quoted
Unfortunately I find it unintuitive and counter productive to perform
inline patches or do anything on a mailing list. Especially on
Windows, it's a pain to setup git to effectively do this. Also I read
mailing lists through Gmail which does not offer a proper monospace
font view or syntax coloring to effectively review patches and
comments pertaining to them.
Are you sure you are not overestimating the effort it takes to send
patches inline? Once you've got your user agent correctly setup its just
a matter of copy and paste instead of attaching the patch. On Windows I
would probably use Thunderbird which has a section in the format-patch
documentation how to configure it. Compared to the effort you probably
spent on writing your patch isn't this bit of extra effort neglectable?
And your patch is almost done. It just needs some tests and maybe a few
rounds on the mailinglist after that.
quoted
Since I am not willing to properly follow your process, I will
withdraw my patch. However it is here if someone else wishes to take
it over. Really wish you guys used github's amazing features but I
understand that Linus has already made his decision in that matter.
It not just Linus decision it is also a matter of many people are used
to this workflow. AFAIR there have been many discussions and tries about
using other tools. Email has many advantages which a webinterface does
not provide. It is simply less effort that one person adjusts to this
workflow instead of changing many peoples working workflow.
quoted
I'm sorry I couldn't be more agreeable on the matter. Thanks for the
time you spent reviewing my patch.
If you are really this fixed in your workflow that would be too bad.
How do you send your patches inline? Do you use git send-email? I have
tried that and it is horrible to setup. Do you just copy/paste the
patch inline in your compose window?

It would be much simpler to fork Git, create a branch, make my change,
and initiate a pull request. I can get email notifications on comments
to my PR diff and address them with subsequent pushes to my branch
(which would also automatically update the code review). Turn around
times for collaborating on a change are much quicker via Github pull
requests.

I am willing to review the typical workflow for contributing via git
on mailing lists but I haven't seen any informative reading material
on this. I just find using command line to email patches and dealing
with other issues not worth the trouble. Lack of syntax highlighting,
lack of monospace font, the fact that I'm basically forced to install
mail client software just to contribute a single git patch.

Re: Diffing submodule does not yield complete logs for merge commits

From: Stefan Beller <hidden>
Date: 2016-06-15 23:04:50

On Tue, May 19, 2015 at 12:29 PM, Robert Dailey
[off-list ref] wrote:
How do you send your patches inline?
There are various ways to do so.
If you look at https://github.com/git/git/blob/master/Documentation/SubmittingPatches
and search for Thunderbird (I used to use Thunderbird for a long time
before switching to
git send-email, so I'll take that as an example) at the bottom:

    Thunderbird, KMail, GMail
    -------------------------

    See the MUA-SPECIFIC HINTS section of git-format-patch(1).

Ok, indirection is the fun part of computers. ;)
So you'd look at the man page of git format patch,
such as here http://git-scm.com/docs/git-format-patch
and scroll the way down to MUA-SPECIFIC HINTS, which offers
3 different ways of doing it. (decisions!)
Do you use git send-email?
I do, but I remember my initial struggle with it (I will contribute only
one patch anyway, so why care?)
I have
tried that and it is horrible to setup. Do you just copy/paste the
patch inline in your compose window?
Once setup correctly git formatpatch / send-email are actually very
convenient (e.g. git send-email HEAD^ --to=git@vger.kernel.org will
just work. And I have strong confidence in it continuing to work,
even when Git decides to revamp the preferred patch format,
line wrapping or other exotic stuff)
It would be much simpler to fork Git, create a branch, make my change,
and initiate a pull request. I can get email notifications on comments
to my PR diff and address them with subsequent pushes to my branch
(which would also automatically update the code review). Turn around
times for collaborating on a change are much quicker via Github pull
requests.
Github has indeed an excellent product, even free for open source.

This workflow discussion was a topic at the GitMerge2015 conference,
and there are essentially 2 groups, those who know how to send email
and those who complain about it. A solution was agreed on by nearly all
of the contributors. It would be awesome to have a git-to-email proxy,
such that you could do a git push <proxy> master:refs/for/mailinglist
and this proxy would convert the push into sending patch series to the
mailing list. It could even convert the following discussion back into
comments (on Github?) but as a first step we'd want to try out a one
way proxy.

Unfortunately nobody stepped up to actually do the work, yet :(
I am willing to review the typical workflow for contributing via git
on mailing lists but I haven't seen any informative reading material
on this. I just find using command line to email patches and dealing
with other issues not worth the trouble. Lack of syntax highlighting,
lack of monospace font, the fact that I'm basically forced to install
mail client software just to contribute a single git patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Diffing submodule does not yield complete logs for merge commits

From: Heiko Voigt <hidden>
Date: 2016-06-15 23:04:51

On Tue, May 19, 2015 at 02:29:55PM -0500, Robert Dailey wrote:
On Tue, May 19, 2015 at 5:44 AM, Heiko Voigt [off-list ref] wrote:
quoted
On Mon, May 18, 2015 at 10:06:32AM -0500, Robert Dailey wrote:
quoted
Unfortunately I find it unintuitive and counter productive to perform
inline patches or do anything on a mailing list. Especially on
Windows, it's a pain to setup git to effectively do this. Also I read
mailing lists through Gmail which does not offer a proper monospace
font view or syntax coloring to effectively review patches and
comments pertaining to them.
Are you sure you are not overestimating the effort it takes to send
patches inline? Once you've got your user agent correctly setup its just
a matter of copy and paste instead of attaching the patch. On Windows I
would probably use Thunderbird which has a section in the format-patch
documentation how to configure it. Compared to the effort you probably
spent on writing your patch isn't this bit of extra effort neglectable?
And your patch is almost done. It just needs some tests and maybe a few
rounds on the mailinglist after that.
quoted
Since I am not willing to properly follow your process, I will
withdraw my patch. However it is here if someone else wishes to take
it over. Really wish you guys used github's amazing features but I
understand that Linus has already made his decision in that matter.
It not just Linus decision it is also a matter of many people are used
to this workflow. AFAIR there have been many discussions and tries about
using other tools. Email has many advantages which a webinterface does
not provide. It is simply less effort that one person adjusts to this
workflow instead of changing many peoples working workflow.
quoted
I'm sorry I couldn't be more agreeable on the matter. Thanks for the
time you spent reviewing my patch.
If you are really this fixed in your workflow that would be too bad.
How do you send your patches inline? Do you use git send-email? I have
tried that and it is horrible to setup. Do you just copy/paste the
patch inline in your compose window?
For bigger patch series I did use send-email but currently I am back to
just using the compose window from whatever email client I am using. On
Windows that would be Thunderbird. But when possible I am not using
Windows.
It would be much simpler to fork Git, create a branch, make my change,
and initiate a pull request. I can get email notifications on comments
to my PR diff and address them with subsequent pushes to my branch
(which would also automatically update the code review). Turn around
times for collaborating on a change are much quicker via Github pull
requests.
I think that depends more on the collaborators than on the tool. When
you get quick replies the turnaround times with both workflows are
quick.

It would be nice if there was a perfect solution for every project that
everyone could use but unfortunately there is not so we sometimes have
to adjust. But I think its more matter of what you are used to. If you
did not have a github account but email software setup you could
complain about the fact that you need to register a github account, fork
git, setup that fork in your local repository, ... instead of just copy
and paste your change into the compose window and then send it to a
mailinglist.
I am willing to review the typical workflow for contributing via git
on mailing lists but I haven't seen any informative reading material
on this. I just find using command line to email patches and dealing
with other issues not worth the trouble. Lack of syntax highlighting,
lack of monospace font, the fact that I'm basically forced to install
mail client software just to contribute a single git patch.
As already mentioned by Stefan there is Documentation/SubmittingPatches
in the Git repository that describes everything and also has a section
on how to do that with Thunderbird.

I tend to not do much on the commandline on Windows since it basically
sucks there. For sending patches you just need

	git format-patch HEAD^

and thats it.

Cheers Heiko

Re: Diffing submodule does not yield complete logs for merge commits

From: Roberto Tyley <hidden>
Date: 2016-06-15 23:04:53

On Tuesday, 19 May 2015, Stefan Beller [off-list ref] wrote:
On Tue, May 19, 2015 at 12:29 PM, Robert Dailey
[off-list ref] wrote:
quoted
How do you send your patches inline?
This workflow discussion was a topic at the GitMerge2015 conference,
and there are essentially 2 groups, those who know how to send email
and those who complain about it. A solution was agreed on by nearly all
of the contributors. It would be awesome to have a git-to-email proxy,
such that you could do a git push <proxy> master:refs/for/mailinglist
and this proxy would convert the push into sending patch series to the
mailing list. It could even convert the following discussion back into
comments (on Github?) but as a first step we'd want to try out a one
way proxy.

Unfortunately nobody stepped up to actually do the work, yet :(
I've replied to this on a separate announcement thread on the Git mailing
list here:

http://thread.gmane.org/gmane.comp.version-control.git/269699

...I've created a new tool called submitGit, which aims to help.
quoted
I am willing to review the typical workflow for contributing via git
on mailing lists but I haven't seen any informative reading material
on this. I just find using command line to email patches and dealing
with other issues not worth the trouble. Lack of syntax highlighting,
lack of monospace font, the fact that I'm basically forced to install
mail client software just to contribute a single git patch.
I'd be interested to know what you think!

Roberto

Re: Diffing submodule does not yield complete logs for merge commits

From: Robert Dailey <hidden>
Date: 2016-06-15 23:05:03

On 5/21/2015 7:51 AM, Heiko Voigt wrote:
On Tue, May 19, 2015 at 02:29:55PM -0500, Robert Dailey wrote:
quoted
On Tue, May 19, 2015 at 5:44 AM, Heiko Voigt [off-list ref] wrote:
quoted
On Mon, May 18, 2015 at 10:06:32AM -0500, Robert Dailey wrote:
quoted
Unfortunately I find it unintuitive and counter productive to perform
inline patches or do anything on a mailing list. Especially on
Windows, it's a pain to setup git to effectively do this. Also I read
mailing lists through Gmail which does not offer a proper monospace
font view or syntax coloring to effectively review patches and
comments pertaining to them.
Are you sure you are not overestimating the effort it takes to send
patches inline? Once you've got your user agent correctly setup its just
a matter of copy and paste instead of attaching the patch. On Windows I
would probably use Thunderbird which has a section in the format-patch
documentation how to configure it. Compared to the effort you probably
spent on writing your patch isn't this bit of extra effort neglectable?
And your patch is almost done. It just needs some tests and maybe a few
rounds on the mailinglist after that.
quoted
Since I am not willing to properly follow your process, I will
withdraw my patch. However it is here if someone else wishes to take
it over. Really wish you guys used github's amazing features but I
understand that Linus has already made his decision in that matter.
It not just Linus decision it is also a matter of many people are used
to this workflow. AFAIR there have been many discussions and tries about
using other tools. Email has many advantages which a webinterface does
not provide. It is simply less effort that one person adjusts to this
workflow instead of changing many peoples working workflow.
quoted
I'm sorry I couldn't be more agreeable on the matter. Thanks for the
time you spent reviewing my patch.
If you are really this fixed in your workflow that would be too bad.
How do you send your patches inline? Do you use git send-email? I have
tried that and it is horrible to setup. Do you just copy/paste the
patch inline in your compose window?
For bigger patch series I did use send-email but currently I am back to
just using the compose window from whatever email client I am using. On
Windows that would be Thunderbird. But when possible I am not using
Windows.
quoted
It would be much simpler to fork Git, create a branch, make my change,
and initiate a pull request. I can get email notifications on comments
to my PR diff and address them with subsequent pushes to my branch
(which would also automatically update the code review). Turn around
times for collaborating on a change are much quicker via Github pull
requests.
I think that depends more on the collaborators than on the tool. When
you get quick replies the turnaround times with both workflows are
quick.

It would be nice if there was a perfect solution for every project that
everyone could use but unfortunately there is not so we sometimes have
to adjust. But I think its more matter of what you are used to. If you
did not have a github account but email software setup you could
complain about the fact that you need to register a github account, fork
git, setup that fork in your local repository, ... instead of just copy
and paste your change into the compose window and then send it to a
mailinglist.
quoted
I am willing to review the typical workflow for contributing via git
on mailing lists but I haven't seen any informative reading material
on this. I just find using command line to email patches and dealing
with other issues not worth the trouble. Lack of syntax highlighting,
lack of monospace font, the fact that I'm basically forced to install
mail client software just to contribute a single git patch.
As already mentioned by Stefan there is Documentation/SubmittingPatches
in the Git repository that describes everything and also has a section
on how to do that with Thunderbird.

I tend to not do much on the commandline on Windows since it basically
sucks there. For sending patches you just need

	git format-patch HEAD^

and thats it.

Cheers Heiko
So I am working on trying to setup my environment (VM through Virtual 
Box) to do some testing on this. You all have encouraged me to try the 
mailing list review model. So I won't give up yet.

In the meantime I'd like to ask, do we even need to add an option for 
this? What if we just make `diff.submodule log` not use --first-parent? 
This seems like a backward compatible change in of itself. And it's 
simpler to implement. I can't think of a good justification to add more 
settings to an already hugely complex configuration scheme for such a 
minor difference in behavior.

Thoughts?

Re: Diffing submodule does not yield complete logs for merge commits

From: Heiko Voigt <hidden>
Date: 2016-06-15 23:05:03

On Fri, May 29, 2015 at 09:18:11PM -0500, Robert Dailey wrote:
So I am working on trying to setup my environment (VM through Virtual Box)
to do some testing on this. You all have encouraged me to try the mailing
list review model. So I won't give up yet.
I am not sure you need a VM or Linux environment. Of course it will be
helpful in case your tests do no pass on Linux (which they sometimes do
due to some differences between the OSes). But until we actually run
into that problem I do not see anything wrong developing your change
purely on Windows. Since it seems you are more familiar with that
platform I would even encourage you to do so. That reduces the toolset
friction which you might experience in a new environment. Even if you
run into the problem, that your tests do not pass on Linux, we might be
able to solve that on the list.

Have you seen the github pull request -> mailing list proxy thing[1]? If
that helps you maybe you can test it and use it for your patch
submission. I think nobody will be annoyed if we get some strange emails
on the list during that testing phase since that might help more
contributors to contribute.
In the meantime I'd like to ask, do we even need to add an option for this?
What if we just make `diff.submodule log` not use --first-parent? This seems
like a backward compatible change in of itself. And it's simpler to
implement. I can't think of a good justification to add more settings to an
already hugely complex configuration scheme for such a minor difference in
behavior.

Thoughts?
This behavior has been with --first-parent for a long time. Even though
it seems like a minor change in my experience there will be complaints
from people that have got used to it and will now get big differences.
You never know how people use it. Since the extra value (full-log)
(AFAIR) has reached a consensus on the list and it allows us to satisfy
both long log and short log users I would prefer to go that route.

Cheers Heiko

[1] http://article.gmane.org/gmane.comp.version-control.git/269699
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help