rev-list --cherry-pick and context lines

rev-list --cherry-pick and context lines

[Stefan Haller]

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/

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

[Michael J Gruber]

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:

- 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) {

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

[Stefan Haller]

Michael J Gruber <git@drmicha.warpmail.net> wrote:

> 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/

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

[Vijay Lakshminarayanan]

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

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

[Stefan Haller]

Vijay Lakshminarayanan <laksvij@gmail.com> wrote:

> 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/

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

[Junio C Hamano]

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.