threads / patch / 11880

patch, 9 partsAdd "skip_unmerged" option to unpack_trees.

Subject: [PATCH 5/9] Add "skip_unmerged" option to unpack_trees.

## tl;dr

6 messages between Feb 4, 2008 and Feb 6, 2008. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Daniel Barkalow· Feb 4, 2008, 18:35 UTC · lore

This option allows the caller to reset everything that isn't unmerged, leaving the unmerged things to be resolved. If, after a merge of "working" and "HEAD", this is used with "HEAD" (reset, !update), the result will be that all of the changes from "local" are in the working tree but not added to the index (either with the index clean but unchanged, or with the index unmerged, depending on whether there are conflicts).

This will be used in checkout -m.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 unpack-trees.c |   20 +++++++++++++++++---
 unpack-trees.h |    1 +
 2 files changed, 18 insertions(+), 3 deletions(-)

Show changes to 2 files +18 −3

unpack-trees.c, unpack-trees.h

diff --git a/unpack-trees.c b/unpack-trees.c
index 40d4130..7040fa3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -85,6 +85,7 @@ static int unpack_trees_rec(struct tree_entry_list **posns, int len,
 		int any_dirs = 0;
 		char *cache_name;
 		int ce_stage;
+		int skip_entry = 0;
 
 		/* Find the first name in the input. */
 
@@ -153,6 +154,8 @@ static int unpack_trees_rec(struct tree_entry_list **posns, int len,
 			any_files = 1;
 			src[0] = active_cache[o->pos];
 			remove = o->pos;
+			if (o->skip_unmerged && ce_stage(src[0]))
+				skip_entry = 1;
 		}
 
 		for (i = 0; i < len; i++) {
@@ -181,6 +184,12 @@ static int unpack_trees_rec(struct tree_entry_list **posns, int len,
 				continue;
 			}
 
+			if (skip_entry) {
+				subposns[i] = df_conflict_list;
+				posns[i] = posns[i]->next;
+				continue;
+			}
+
 			if (!o->merge)
 				ce_stage = 0;
 			else if (i + 1 < o->head_idx)
@@ -205,7 +214,13 @@ static int unpack_trees_rec(struct tree_entry_list **posns, int len,
 			posns[i] = posns[i]->next;
 		}
 		if (any_files) {
-			if (o->merge) {
+			if (skip_entry) {
+				do
+					o->pos++;
+				while (o->pos < active_nr &&
+				       !strcmp(active_cache[o->pos]->name,
+					       src[0]->name));
+			} else if (o->merge) {
 				int ret;
 
 #if DBRT_DEBUG > 1
@@ -730,9 +745,8 @@ int threeway_merge(struct cache_entry **stages,
 	 * If we have an entry in the index cache, then we want to
 	 * make sure that it matches head.
 	 */
-	if (index && !same(index, head)) {
+	if (index && !same(index, head))
 		return o->gently ? -1 : reject_merge(index);
-	}
 
 	if (head) {
 		/* #5ALT, #15 */
diff --git a/unpack-trees.h b/unpack-trees.h
index 83d1229..a2df544 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -16,6 +16,7 @@ struct unpack_trees_options {
 	int trivial_merges_only;
 	int verbose_update;
 	int aggressive;
+	int skip_unmerged;
 	int gently;
 	const char *prefix;
 	int pos;

-- 
1.5.4

Johannes Schindelin· Feb 5, 2008, 01:32 UTC · re: Daniel Barkalow · lore

Re: [PATCH 5/9] Add "skip_unmerged" option to unpack_trees.

Hi,

On Mon, 4 Feb 2008, Daniel Barkalow wrote:

Show 9 quoted lines

>  		if (any_files) {
> -			if (o->merge) {
> +			if (skip_entry) {
> +				do
> +					o->pos++;
> +				while (o->pos < active_nr &&
> +				       !strcmp(active_cache[o->pos]->name,
> +					       src[0]->name));
> +			} else if (o->merge) {

Maybe it is just me, but I would have thought

				while (++o->pos < active_nr)
					if (strcmp(active_cache[o->pos]->name,
							src[0]->name))
						break;

more readable. But that's maybe because I have trouble with do ... while constructs logically (I like to see the loop condition first, then the loop body).

Ciao, Dscho

Daniel Barkalow· Feb 5, 2008, 20:38 UTC · re: Johannes Schindelin · lore

Re: [PATCH 5/9] Add "skip_unmerged" option to unpack_trees.

On Tue, 5 Feb 2008, Johannes Schindelin wrote:

Show 25 quoted lines

> Hi,
> 
> On Mon, 4 Feb 2008, Daniel Barkalow wrote:
> 
> 
> >  		if (any_files) {
> > -			if (o->merge) {
> > +			if (skip_entry) {
> > +				do
> > +					o->pos++;
> > +				while (o->pos < active_nr &&
> > +				       !strcmp(active_cache[o->pos]->name,
> > +					       src[0]->name));
> > +			} else if (o->merge) {
> 
> Maybe it is just me, but I would have thought
> 
> 				while (++o->pos < active_nr)
> 					if (strcmp(active_cache[o->pos]->name,
> 							src[0]->name))
> 						break;
> 
> more readable.  But that's maybe because I have trouble with do ... while 
> constructs logically (I like to see the loop condition first, then the 
> loop body).

I find yours less readable, because the loop condition is an exceptional case (this is the last entry, so we run out of active_cache before finding anything else), and you've got the actual effect of the loop in the condition instead of the body, and I find using the value of ++x or x++ a bit confusing outside of regular idioms. I'd go for:

				o->pos++;
				while (o->pos < active_nr &&
				       !strcmp(active_cache[o->pos]->name,
					       src[0]->name))
					o->pos++;

if you care, though; it's not bad to make it clear we're skipping the first of these entries based on a different consideration from the rest (the first is the entry we decided to skip, and the rest are ones that match it in filename).

	-Daniel
*This .sig left intentionally blank*

Johannes Schindelin· Feb 5, 2008, 22:50 UTC · re: Daniel Barkalow · lore

Re: [PATCH 5/9] Add "skip_unmerged" option to unpack_trees.

Hi,

On Tue, 5 Feb 2008, Daniel Barkalow wrote:

Show 42 quoted lines

> On Tue, 5 Feb 2008, Johannes Schindelin wrote:
> 
> > On Mon, 4 Feb 2008, Daniel Barkalow wrote:
> > 
> > 
> > >  		if (any_files) {
> > > -			if (o->merge) {
> > > +			if (skip_entry) {
> > > +				do
> > > +					o->pos++;
> > > +				while (o->pos < active_nr &&
> > > +				       !strcmp(active_cache[o->pos]->name,
> > > +					       src[0]->name));
> > > +			} else if (o->merge) {
> > 
> > Maybe it is just me, but I would have thought
> > 
> > 				while (++o->pos < active_nr)
> > 					if (strcmp(active_cache[o->pos]->name,
> > 							src[0]->name))
> > 						break;
> > 
> > more readable.  But that's maybe because I have trouble with do ... 
> > while constructs logically (I like to see the loop condition first, 
> > then the loop body).
> 
> I find yours less readable, because the loop condition is an exceptional 
> case (this is the last entry, so we run out of active_cache before 
> finding anything else), and you've got the actual effect of the loop in 
> the condition instead of the body, and I find using the value of ++x or 
> x++ a bit confusing outside of regular idioms. I'd go for:
> 
> 				o->pos++;
> 				while (o->pos < active_nr &&
> 				       !strcmp(active_cache[o->pos]->name,
> 					       src[0]->name))
> 					o->pos++;
> 
> if you care, though; it's not bad to make it clear we're skipping the 
> first of these entries based on a different consideration from the rest 
> (the first is the entry we decided to skip, and the rest are ones that 
> match it in filename).

Well, I still find mine more readable (because it reflects how I think, I guess), but I do not care deeply enough. Let's take your original.

Ciao, Dscho

Junio C Hamano· Feb 5, 2008, 23:44 UTC · re: Daniel Barkalow · lore

Re: [PATCH 5/9] Add "skip_unmerged" option to unpack_trees.

Daniel Barkalow <barkalow@iabervon.org> writes:

Show 44 quoted lines

> On Tue, 5 Feb 2008, Johannes Schindelin wrote:
>
>> Hi,
>> 
>> On Mon, 4 Feb 2008, Daniel Barkalow wrote:
>> 
>> 
>> >  		if (any_files) {
>> > -			if (o->merge) {
>> > +			if (skip_entry) {
>> > +				do
>> > +					o->pos++;
>> > +				while (o->pos < active_nr &&
>> > +				       !strcmp(active_cache[o->pos]->name,
>> > +					       src[0]->name));
>> > +			} else if (o->merge) {
>> 
>> Maybe it is just me, but I would have thought
>> 
>> 				while (++o->pos < active_nr)
>> 					if (strcmp(active_cache[o->pos]->name,
>> 							src[0]->name))
>> 						break;
>> 
>> more readable.  But that's maybe because I have trouble with do ... while 
>> constructs logically (I like to see the loop condition first, then the 
>> loop body).
>
> I find yours less readable, because the loop condition is an exceptional 
> case (this is the last entry, so we run out of active_cache before finding 
> anything else), and you've got the actual effect of the loop in the 
> condition instead of the body, and I find using the value of ++x or x++ a 
> bit confusing outside of regular idioms. I'd go for:
>
> 				o->pos++;
> 				while (o->pos < active_nr &&
> 				       !strcmp(active_cache[o->pos]->name,
> 					       src[0]->name))
> 					o->pos++;
>
> if you care, though; it's not bad to make it clear we're skipping the 
> first of these entries based on a different consideration from the rest 
> (the first is the entry we decided to skip, and the rest are ones that 
> match it in filename).

The last one is vastly more readable than your original, and moderately easier to follow than Dscho's, at least to me.

Thanks.

Daniel Barkalow· Feb 6, 2008, 01:05 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/9] Add "skip_unmerged" option to unpack_trees.

On Tue, 5 Feb 2008, Junio C Hamano wrote:

Show 49 quoted lines

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Tue, 5 Feb 2008, Johannes Schindelin wrote:
> >
> >> Hi,
> >> 
> >> On Mon, 4 Feb 2008, Daniel Barkalow wrote:
> >> 
> >> 
> >> >  		if (any_files) {
> >> > -			if (o->merge) {
> >> > +			if (skip_entry) {
> >> > +				do
> >> > +					o->pos++;
> >> > +				while (o->pos < active_nr &&
> >> > +				       !strcmp(active_cache[o->pos]->name,
> >> > +					       src[0]->name));
> >> > +			} else if (o->merge) {
> >> 
> >> Maybe it is just me, but I would have thought
> >> 
> >> 				while (++o->pos < active_nr)
> >> 					if (strcmp(active_cache[o->pos]->name,
> >> 							src[0]->name))
> >> 						break;
> >> 
> >> more readable.  But that's maybe because I have trouble with do ... while 
> >> constructs logically (I like to see the loop condition first, then the 
> >> loop body).
> >
> > I find yours less readable, because the loop condition is an exceptional 
> > case (this is the last entry, so we run out of active_cache before finding 
> > anything else), and you've got the actual effect of the loop in the 
> > condition instead of the body, and I find using the value of ++x or x++ a 
> > bit confusing outside of regular idioms. I'd go for:
> >
> > 				o->pos++;
> > 				while (o->pos < active_nr &&
> > 				       !strcmp(active_cache[o->pos]->name,
> > 					       src[0]->name))
> > 					o->pos++;
> >
> > if you care, though; it's not bad to make it clear we're skipping the 
> > first of these entries based on a different consideration from the rest 
> > (the first is the entry we decided to skip, and the rest are ones that 
> > match it in filename).
> 
> The last one is vastly more readable than your original, and
> moderately easier to follow than Dscho's, at least to me.

I like this version best, too, and it's what I've got in my revised series.

	-Daniel
*This .sig left intentionally blank*