threads / discuss / 3147

What's in git.git

Subject: What's in git.git

## tl;dr

6 messages between Jan 25, 2006 and Jan 26, 2006.

replies: 5people: 3as markdown or json

Junio C Hamano· Jan 25, 2006, 13:00 UTC · lore
I've pushed out the following changes to "master" branch:
 * git-clone
   . --naked option is still accepted but is deprecated.  Please
     say --bare.
   . a repository cloned with --bare does not get "origin"
     branch, nor remotes/origin file.
   . Earlier we accepted more than one -o options, without
     complaining.  We now complain.
 * fetch and peek/ls-remote (Michal Ostrowski)
   . --upload-pack option to the underlying git-peek-remote and
     friends can be passed from the barebone Porcelain.
 * local push/pull environment fix (Matt Draisey)
   . We now clean "GIT_DIR" and friends from the environment
     when spawning a program on the other end to drive git
     native protocols on a local machine.  This uses unsetenv(),
     which is not strictly portable, but I was too lazy to fix
     it myself.  I am hoping that Jason Riedy will scream and
     give us a patch to make it work again on his Solaris box
     ;-).
 * sample update-hook rewrite (Andreas Ericsson)
 * asciidoc --unsafe workaround (Pavel Roskin)

In the "pu" branch, there are some interesting changes. The most immediately visible usability enhancement is the "combined diff" option to git-diff-tree command. Interested people can try something like this:

	$ git checkout pu
	$ make clean strip all install
	$ git checkout master

Do not forget to come back to your "master" branch once you are done, or your next pull would be screwed.

        $ git whatchanged --cc --abbrev pu

This would show an improved version of "git whatchanged -m -p". The difference is that it uses the "dense combined diff" suggested by Linus yesterday. Commits near the tip of "pu" tend to be Octopus, and you would see quite interesting combined diffs for them. I am not proud of the implementation itself, and I am sure there are more bugs to be discovered, but overall I am reasonably happy with what it shows. Once it stabilizes, it would be a good addition to gitweb UI. In addition to the "commitdiff" next to each parent, there would be an extra link in a merge commit to get a combined diff.

The "annotate helper" change by Linus and "bound commit" subproject support experiments are there as before.

There are other minor fixes and enhancements. Not all of them may make it to "master":

 * "diff-tree --abbrev --pretty" was still showing full 40
   characters of commit object names on "Merge:" line.
 * "rev-parse --abbrev[=<n>] $rev" is similar to
   "rev-parse --verify $rev", except that it shows an
   abbreviated object name.
 * The flag argument was handled confusingly by "rev-parse
   [--flags|--no-flags]".  The command now treats anything that
   come after --flags/--no-flags things to be parsed, not an
   option to control what the command does.  This was primarily
   needed to allow parsing out "--abbrev" option and giving it
   to "diff-tree" inside whatchanged.  All existing users have
   been adjusted to this change.  I do not think any of Cogito,
   gitk, nor stgit is affected.
Jason Riedy· Jan 25, 2006, 20:36 UTC · re: Junio C Hamano · lore

Re: What's in git.git

And Junio C Hamano writes:
 -      This uses unsetenv(), which is not strictly portable, but 
 - 	I was too lazy to fix it myself.

And you manage to catch me on the one day in the last month I've played with git code... Patch to add compat/unsetenv.c coming shortly. Passes unit tests and make test as well as before (I have some wierd, local-only cpio problems), but I haven't used this extensively.

People _with_ unsetenv can still add compat functions, you know. ;)

Jason
Junio C Hamano· Jan 25, 2006, 19:24 UTC · lore

Re: What's in git.git

Santi Bejar <sbejar@gmail.com> writes:
> I think we should be consistent with respect the leading dots. Now the
> diff-tree line has the dots, but the merge line do not.

This is deliberate. Merge lines do not have to align with anything else for readability.

Junio C Hamano· Jan 25, 2006, 23:56 UTC · lore

Re: What's in git.git

Santi Bejar <sbejar@gmail.com> writes:
Show 7 quoted lines
> Junio C Hamano <junkio@cox.net> writes:
>...
>>         $ git whatchanged --cc --abbrev pu
>...
>       Then some comments :)
>
>       * There is an extra space between the +- and the content.
Thanks for noticing.  Fixed.
>       * I think the "diff command" could be just "diff --git".  And
>         "diff --git --cc" for the "dense combined diff".

I chose not say "--git" because I wanted to make sure people would not run "git apply" by mistake. I replaced them with "--combined" and "--cc" to make the differences clearer.

>       * I miss the index lines

That would probably be easy to add in the later rounds if you really want them, but I consider it lower priority for now. This is designed to be human readable and not necessarily machine applicable, so I do not see much point in showing them.

Mode changes, creation, deletion, rename, and copy may need to be there, though. The code currently punts on them. Patches welcome.

Show 24 quoted lines
>       * In the case of 3 and more parents I find uninteresting the hunk
>         where the merge is equal to one (or more) of its children and
>         the rest are equal. For example the first hunk in 22573dd, where
>         all the children are equal expect the picked one:
>
> diff-tree 22573dd... (from parents)
> Merge: 92643a2 0359fe8 a428965 4aa079f
> Author: Junio C Hamano <junkio@cox.net>
> Date:   Wed Jan 25 03:51:37 2006 -0800
>
>     Merge lt/revlist,jc/diff,jc/bind
>
> diff --combined rev-list.c
> @@@@@ +37,7 @@@@@
>      static int dense = 1;
>      static int unpacked = 0;
>      static int bisect_list = 0;
> ---  static int tag_objects = 0;
> ---  static int tree_objects = 0;
> ---  static int blob_objects = 0;
> +++  static int list_objects = 0;
>      static int verbose_header = 0;
>      static int show_parents = 0;
>      static int hdr_termination = 0;

I noticed it and found it somewhat less interesting than others, but left it the way. It is "changed the same way from all of these parents except this one", which is different from what I culled in the version you are commenting on, i.e. "changed only from one parent".

I've since updated the logic to drop these hunks as well. Please take a look at the tip of "pu".

>       * Like you said in another thread, the line numbers of all the
>         files.
This is left as an exercise for the reader ;-)
Show 6 quoted lines
> So, at the end, I suggest this output for the diff:
>...
> and this for the diff-raw:
>
> :100644 100644 56505b4... 538d21d... M  Makefile
> :100644 100644 30479b4... 538d21d... M  Makefile

I do not find this to be so interesting. diff-raw is primarily for quick sanity check and machine processing, so I'd rather not play games when generating diff-raw in order to keep the latter form of users sane.

Junio C Hamano· Jan 26, 2006, 12:12 UTC · lore

Re: What's in git.git

Santi Bejar <sbejar@gmail.com> writes:
Show 10 quoted lines
> 2006/1/26, Junio C Hamano <junkio@cox.net>:
>> No comment on your message yet, but please fix your MUA so that
>> your messages do not get rejected by the mailing list software.
>>
>> I suspect it is base64 CTE that causes it.
>>
>>         Content-Type: text/plain; charset=UTF-8
>>         Content-Transfer-Encoding: base64
>
> Sorry, about that. But I didn't get any notification.

You do not need to feel sorry. It's just your voice is not heard as widely as it should have been, and you did not know about it.

The spam filter just drops things on the floor without bothering to notify potential spammers.

I think this patch would address your two issues (applies on top of "pu"), but I have not had time to test it seriously enough.

-- >8 -- [PATCH] combine-diff: better hunk splitting.

It considered an otherwise unchanged line that had line removals in front of it an interesting line, which caused hunks to have one extra the trailing context line.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 combine-diff.c |  142 ++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 116 insertions(+), 26 deletions(-)
0aa20bc0c276c3fcb7215ba9f591960178792261
diff --git a/combine-diff.c b/combine-diff.c
index 3b219a0..d94a93d 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -323,52 +323,142 @@ static unsigned long line_all_diff(struc
 	return different;
 }
 
-static int make_hunks(struct sline *sline, unsigned long cnt,
-		       int num_parent, int dense)
+static unsigned long adjust_hunk_tail(struct sline *sline,
+				      unsigned long all_mask,
+				      unsigned long hunk_begin,
+				      unsigned long i)
+{
+	/* i points at the first uninteresting line.
+	 * If the last line of the hunk was interesting
+	 * only because it has some deletion, then
+	 * it is not all that interesting for the
+	 * purpose of giving trailing context lines.
+	 */
+	if ((hunk_begin + 1 <= i) &&
+	    ((sline[i-1].flag & all_mask) == all_mask))
+		i--;
+	return i;
+}
+
+static unsigned long next_interesting(struct sline *sline,
+				      unsigned long mark,
+				      unsigned long i,
+				      unsigned long cnt,
+				      int uninteresting)
+{
+	while (i < cnt)
+		if (uninteresting ?
+		    !(sline[i].flag & mark) :
+		    (sline[i].flag & mark))
+			return i;
+		else
+			i++;
+	return cnt;
+}
+
+static int give_context(struct sline *sline, unsigned long cnt, int num_parent)
 {
 	unsigned long all_mask = (1UL<<num_parent) - 1;
 	unsigned long mark = (1UL<<num_parent);
 	unsigned long i;
 	int has_interesting = 0;
 
-	i = 0;
+	i = next_interesting(sline, mark, 0, cnt, 0);
+	if (cnt <= i)
+		return 0;
+
 	while (i < cnt) {
-		if (interesting(&sline[i], all_mask)) {
-			unsigned long j = (context < i) ? i - context : 0;
-			while (j <= i)
+		unsigned long j = (context < i) ? (i - context) : 0;
+		unsigned long k;
+		while (j < i)
+			sline[j++].flag |= mark;
+
+	again:
+		j = next_interesting(sline, mark, i, cnt, 1);
+		if (cnt <= j)
+			break; /* the rest are all interesting */
+
+		/* lookahead context lines */
+		k = next_interesting(sline, mark, j, cnt, 0);
+		j = adjust_hunk_tail(sline, all_mask, i, j);
+
+		if (k < j + context) {
+			/* k is interesting and [j,k) are not, but
+			 * paint them interesting because the gap is small.
+			 */
+			while (j < k)
 				sline[j++].flag |= mark;
-			while (++i < cnt) {
-				if (!interesting(&sline[i], all_mask))
-					break;
-				sline[i].flag |= mark;
-			}
-			j = (i + context < cnt) ? i + context : cnt;
-			while (i < j)
-				sline[i++].flag |= mark;
-			has_interesting = 1;
-			continue;
+			i = k;
+			goto again;
 		}
-		i++;
+
+		/* j is the first uninteresting line and there is
+		 * no overlap beyond it within context lines.
+		 */
+		i = k;
+		k = (j + context < cnt) ? j + context : cnt;
+		while (j < k)
+			sline[j++].flag |= mark;
+	}
+	return 1;
+}
+
+static int make_hunks(struct sline *sline, unsigned long cnt,
+		       int num_parent, int dense)
+{
+	unsigned long all_mask = (1UL<<num_parent) - 1;
+	unsigned long mark = (1UL<<num_parent);
+	unsigned long i;
+	int has_interesting = 0;
+
+	for (i = 0; i < cnt; i++) {
+		if (interesting(&sline[i], all_mask))
+			sline[i].flag |= mark;
+		else
+			sline[i].flag &= ~mark;
 	}
 	if (!dense)
-		return has_interesting;
+		return give_context(sline, cnt, num_parent);
 
 	/* Look at each hunk, and if we have changes from only one
 	 * parent, or the changes are the same from all but one
 	 * parent, mark that uninteresting.
 	 */
-	has_interesting = 0;
 	i = 0;
 	while (i < cnt) {
-		int j, hunk_end, same, diff;
+		unsigned long j, hunk_begin, hunk_end;
+		int same, diff;
 		unsigned long same_diff, all_diff;
 		while (i < cnt && !(sline[i].flag & mark))
 			i++;
 		if (cnt <= i)
 			break; /* No more interesting hunks */
-		for (hunk_end = i + 1; hunk_end < cnt; hunk_end++)
-			if (!(sline[hunk_end].flag & mark))
-				break;
+		hunk_begin = i;
+		for (j = i + 1; j < cnt; j++) {
+			if (!(sline[j].flag & mark)) {
+				/* Look beyond the end to see if there
+				 * is an interesting line after this
+				 * hunk within context span.
+				 */
+				unsigned long la; /* lookahead */
+				int contin = 0;
+				la = adjust_hunk_tail(sline, all_mask,
+						     hunk_begin, j);
+				la = (la + context < cnt) ?
+					(la + context) : cnt;
+				while (j <= --la) {
+					if (sline[la].flag & mark) {
+						contin = 1;
+						break;
+					}
+				}
+				if (!contin)
+					break;
+				j = la;
+			}
+		}
+		hunk_end = j;
+
 		/* [i..hunk_end) are interesting.  Now does it have
 		 * the same change with all but one parent?
 		 */
@@ -387,13 +477,13 @@ static int make_hunks(struct sline *slin
 		}
 		if ((num_parent - 1 <= same) || (diff == 1)) {
 			/* This hunk is not that interesting after all */
-			for (j = i; j < hunk_end; j++)
+			for (j = hunk_begin; j < hunk_end; j++)
 				sline[j].flag &= ~mark;
 		}
-		else
-			has_interesting = 1;
 		i = hunk_end;
 	}
+
+	has_interesting = give_context(sline, cnt, num_parent);
 	return has_interesting;
 }
 

-- 
1.1.4.g2cff
Santi Bejar· Jan 26, 2006, 16:24 UTC · re: Junio C Hamano · lore

Re: What's in git.git

> I think this patch would address your two issues (applies on top
> of "pu"), but I have not had time to test it seriously enough.
It works here, thanks.

Just a little issue. With grafted commits the combined diff works but the "Merge:" line is not shown.

Santi

← back to recent threads