Re: [PATCH] git log -p -m: Document, honor --first-parent

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Petr Baudis <pasky@xxxxxxx> writes:

> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 0e39bb6..a2a2d04 100644
> --- a/Documentation/git-log.txt
> +++ b/Documentation/git-log.txt
> @@ -118,6 +118,15 @@ git log master --not --remotes=*/master::
>  	Shows all commits that are in local master but not in any remote
>  	repository master branches.
>  
> +git log -p -m --first-parent::
> +
> +	Shows the history including change diffs, but only from the
> +	"main branch" perspective, skipping commits that come only from
> +	merges, and showing full diffs of changes introduced by the merges.
> +	This makes sense only when following a strict policy of merging all
> +	topic branches when staying on a single integration branch and
> +	making sure the merges are not fast-forwards.

I think the tone of the last three lines is too strong.

Why is it necessary to make a merge with a single commit side branch when
fast-forward would do?  And if the side branch is actually two or more
commits, it will show the broken-down changes in more detail, but the fact
that it was made on the "primary" history would also have some
significance (e.g. trivial and obvious fixes made directly on 'master',
other branches merged from topic after cooking).

It is Ok to elaborate on the "policy" issues in the Discussion section,
but otherwise, I would rather see you spend the same number of lines to
clarify "showing full diffs of changes introduced by the merges" a bit
better (e.g. it is unclear if you are showing diff from each parents or
just from the first parent).  Perhaps "s/introduced /& to the first-parent
ancestry /" may suffice.

> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 6e9baf8..d7d0dee 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -108,8 +108,8 @@ options may be given. See linkgit:git-diff-files[1] for more options.
>  
>  -c::
>  
> -	This flag changes the way a merge commit is displayed.  It shows
> -	the differences from each of the parents to the merge result
> +	This flag forces the default way a merge commit is displayed.  It
> +	shows the differences from each of the parents to the merge result
>  	simultaneously instead of showing pairwise diff between a parent

Sorry, I don't understand this change; "forces the default?"  Any option
"forces" the command to behave differently.  At least the original is
understandable "Ah, without it it shows one way but with this it shows in
a different way", even though that does not carry much useful information
(i.e. what are the two ways?  ah, I need to read further down).

> diff --git a/log-tree.c b/log-tree.c
> index 27afcf6..fb990a1 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -514,6 +514,14 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
>  			return 0;
>  		else if (opt->combine_merges)
>  			return do_diff_combined(opt, commit);
> +		else if (opt->first_parent_only) {
> +			/* Generate merge log entry only for the first
> +			 * parent, showing summary diff of the others
> +			 * we merged _in_. */

Style?

Don't we use --cc as default for "show" (and possibly "log"---I don't
remember the details)?

> +			diff_tree_sha1(parents->item->object.sha1, sha1, "", &opt->diffopt);
> +			log_tree_diff_flush(opt);
> +			return !opt->loginfo;
> +		}

This needs some tests but I think it is a good first step in the right
direction.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]