threads / discuss / 28285

rev-list --cherry-pick and context lines

Subject: rev-list --cherry-pick and context lines

## tl;dr

6 messages between Sep 2, 2011 and Sep 2, 2011.

replies: 5people: 4as markdown or json

Stefan Haller· Sep 2, 2011, 10:35 UTC · lore
Consider two commits on different branches, one with this patch:
    diff --git a/file.txt b/file.txt
    index 704fa27..2f7e74c 100644
    --- a/file.txt
    +++ b/file.txt
    @@ -1,3 +1,3 @@
     old_context
     
    -foo
    +bar
and the other with this patch:
    diff --git a/file.txt b/file.txt
    index f35051b..8c7de32 100644
    --- a/file.txt
    +++ b/file.txt
    @@ -1,3 +1,3 @@
     new_context
     
    -foo
    +bar

If I run "git rev-list --cherry-pick --left-right branch1...branch2", it reports both commits as being genuine commits on their respective branch, even though I consider their patches to be the same.

I guess for my purpose I would like to have patch-ids that ignore context (or that use only one line of context, I'm not sure which).

In fact, if I do "git show <commit> -U1 | git patch-id", both commits show the same id.

So, would it make sense to have a parameter for git-rev-list (and git-cherry) that lets you specify how much context to be used for the patch ids?

-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/
Michael J Gruber· Sep 2, 2011, 15:32 UTC · re: Stefan Haller · lore

Re: rev-list --cherry-pick and context lines

Stefan Haller venit, vidit, dixit 02.09.2011 12:35:
Show 37 quoted lines
> Consider two commits on different branches, one with this patch:
> 
>     diff --git a/file.txt b/file.txt
>     index 704fa27..2f7e74c 100644
>     --- a/file.txt
>     +++ b/file.txt
>     @@ -1,3 +1,3 @@
>      old_context
>      
>     -foo
>     +bar
> 
> and the other with this patch:
> 
>     diff --git a/file.txt b/file.txt
>     index f35051b..8c7de32 100644
>     --- a/file.txt
>     +++ b/file.txt
>     @@ -1,3 +1,3 @@
>      new_context
>      
>     -foo
>     +bar
> 
> If I run "git rev-list --cherry-pick --left-right branch1...branch2", it
> reports both commits as being genuine commits on their respective
> branch, even though I consider their patches to be the same.
> 
> I guess for my purpose I would like to have patch-ids that ignore
> context (or that use only one line of context, I'm not sure which).
> 
> In fact, if I do "git show <commit> -U1 | git patch-id", both commits
> show the same id.
> 
> So, would it make sense to have a parameter for git-rev-list (and
> git-cherry) that lets you specify how much context to be used for the
> patch ids?

It would be a bit like the patch below. "git log" accepts diff options already. But:

- Do we want the patch id generation and the patch display (-p) to use the
  same options?
- -U1 implies -p/--patch and there is no --no-patch.
- Which other diff options do we want to pass to the patch id
  generation: --histogram, --patience, ...?

Cheers, Michael

----
diff --git i/diff.c w/diff.c
index fcc0078..4e82912 100644
--- i/diff.c
+++ w/diff.c
@@ -4103,7 +4103,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 		}
 
 		xpp.flags = 0;
-		xecfg.ctxlen = 3;
+		xecfg.ctxlen = options->context;
 		xecfg.flags = 0;
 		xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,
 			      &xpp, &xecfg);
diff --git i/revision.c w/revision.c
index 072ddac..5a98ed9 100644
--- i/revision.c
+++ w/revision.c
@@ -601,6 +601,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 	left_first = left_count < right_count;
 	init_patch_ids(&ids);
 	ids.diffopts.pathspec = revs->diffopt.pathspec;
+	ids.diffopts.context = revs->diffopt.context;
 
 	/* Compute patch-ids for one side */
 	for (p = list; p; p = p->next) {
Stefan Haller· Sep 2, 2011, 16:33 UTC · re: Michael J Gruber · lore

Re: rev-list --cherry-pick and context lines

Michael J Gruber <git@drmicha.warpmail.net> wrote:
Show 42 quoted lines
> Stefan Haller venit, vidit, dixit 02.09.2011 12:35:
> > Consider two commits on different branches, one with this patch:
> > 
> >     diff --git a/file.txt b/file.txt
> >     index 704fa27..2f7e74c 100644
> >     --- a/file.txt
> >     +++ b/file.txt
> >     @@ -1,3 +1,3 @@
> >      old_context
> >      
> >     -foo
> >     +bar
> > 
> > and the other with this patch:
> > 
> >     diff --git a/file.txt b/file.txt
> >     index f35051b..8c7de32 100644
> >     --- a/file.txt
> >     +++ b/file.txt
> >     @@ -1,3 +1,3 @@
> >      new_context
> >      
> >     -foo
> >     +bar
> > 
> > If I run "git rev-list --cherry-pick --left-right branch1...branch2", it
> > reports both commits as being genuine commits on their respective
> > branch, even though I consider their patches to be the same.
> > 
> > I guess for my purpose I would like to have patch-ids that ignore
> > context (or that use only one line of context, I'm not sure which).
> > 
> > In fact, if I do "git show <commit> -U1 | git patch-id", both commits
> > show the same id.
> > 
> > So, would it make sense to have a parameter for git-rev-list (and
> > git-cherry) that lets you specify how much context to be used for the
> > patch ids?
> 
> It would be a bit like the patch below. "git log" accepts diff options already.
> But:
> [...]

Thanks a lot. I can't contribute much to answering your "But:" questions; I can only add more questions myself. :-)

Is there a reason why the hard-coded default is 3 in the current code? It seems to me that 1 would be a better choice; it would mean "patches are equal if their added/removed lines are the same, and they could be cherry-picked without conflicts."

Now, I'm in a situation where I'll be stuck with git 1.7.1 for quite a while, so no patch is going to help me. It looks like the only way to get the behaviour I want is to reimplement git-rev-list --cherry-pick myself, feeding each patch to git-patch-id, right? (Horribly inefficient, but might be good enough for my purpose. Just wondering if I'm missing a smarter way to solve it.)

Thanks,
   Stefan
-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/
Vijay Lakshminarayanan· Sep 2, 2011, 18:14 UTC · re: Stefan Haller · lore

Re: rev-list --cherry-pick and context lines

lists@haller-berlin.de (Stefan Haller) writes:
> Is there a reason why the hard-coded default is 3 in the current code?

3 is the default context for GNU diff (and possibly other diff implementations also).

> Thanks,
>    Stefan
-- 
Cheers
~vijay
Stefan Haller· Sep 2, 2011, 18:45 UTC · re: Vijay Lakshminarayanan · lore

Re: rev-list --cherry-pick and context lines

Vijay Lakshminarayanan <laksvij@gmail.com> wrote:
Show 6 quoted lines
> lists@haller-berlin.de (Stefan Haller) writes:
> 
> > Is there a reason why the hard-coded default is 3 in the current code?
> 
> 3 is the default context for GNU diff (and possibly other diff
> implementations also).

I think you misunderstood my question; I realize that 3 is a useful default for "git show" or "git log -p" et al, for the reason you give.

My question was why 3 is the hard-coded value for calculating patch-ids. For that case, 1 seems to make more sense to me.

-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/
Junio C Hamano· Sep 2, 2011, 19:13 UTC · re: Stefan Haller · lore

Re: rev-list --cherry-pick and context lines

lists@haller-berlin.de (Stefan Haller) writes:
> Is there a reason why the hard-coded default is 3 in the current code?
> It seems to me that 1 would be a better choice; it would mean "patches
> are equal if their added/removed lines are the same, and they could be
> cherry-picked without conflicts."

Even if two patches have the same added/deleted lines, depending on where in the preimage they are applied (which is given by the context lines), the meaning of the patches can be and are very different. So if the default for showing uses 3-line contect (hence applying, as e-mailed patches are usually generated with 3-line context), it makes sense for the default to generate patch id to match it.

As Michael hinted, I think it would make sense to reduce the number of context lines to generate patch-id the same way if/when you reduce the context to smaller number of lines for the purpose of patch application, but it does not make sense to use zero-line context as default.

← back to recent threads