Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-29 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 On Fri, Jul 26, 2013 at 05:40:36PM -0400, Jeff King wrote:

  Jeff King wrote:
  
   Your patch is just swapping out git reset for cherry-pick --abort,
   so I think that is a good improvement in the meantime.
  
  Um, wasn't the idea of the original message that you can run git
  reset and then git cherry-pick --continue?
 
 Maybe. :)
 
 I missed that subtlety. Of my three things you would want to do, that
 means it was _trying_ say number 2, how to skip, rather than 3, how to
 abort. If that is the case, then it should probably explain the sequence
 of steps as reset and then --continue to make it more clear.
 
 I.e., a patch is needed, but Ram's is going in the opposite direction.

 I played around a bit with the test cases that Ram showed. It seems like
 the advice needed is different depending on whether you are in a single
 or multi-commit cherry-pick.

 So if we hit an empty commit and you want to:

   1. Make an empty commit, then always run git commit --allow-empty.

   2. Skip this commit, then if:

  a. this is a single commit cherry-pick, you run git reset (and
 nothing more, the cherry pick is finished; running cherry-pick
 --continue will yield an error).

Yes, the single-replay mode never required cherry-pick --continue
to clean sequencer cruft when discarding a failed cherry-pick, so it
is a natural consequence of a conscious design decision that
cherry-pick --continue will say you are not running a
cherry-pick, exactly because you no longer are.

  b. this is a multi-commit cherry-pick, you run git reset,
 followed by git cherry-pick --continue

True.

   3. Abort the commit, run git cherry-pick --abort

 Let's assume that the instructions we want to give the user are how to
 do options 1 and 2. I do not mind omitting 3, as it should be reasonably
 obvious that cherry-pick --abort is always good way to abort.

 So we give good instructions for the single-commit case, but bad
 instructions for the multi-commit case.

Yeah, that matches what I thought.  It appears that when we did a
shoddy job when teaching commit to give this advice-message and only
considered a single-pick mode, perhaps because multi-replay mode was
relatively new back then.

 I think instead we would want to leave the single-commit case alone, and
 for the multi-commit case add ...and then cherry-pick --continue. That
 message is generated from within git-commit, though; I guess it would
 need to learn about the difference between single/multi cherry-picks.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-29 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 Here it is in patch form, with an updated commit message that hopefully
 explains the rationale a bit better.

 -- 8 --
 Subject: [PATCH] commit: tweak empty cherry pick advice for sequencer

 When we refuse to make an empty commit, we check whether we
 are in a cherry-pick in order to give better advice on how
 to proceed. We instruct the user to repeat the commit with
 --allow-empty to force the commit, or to use git reset
 to skip it and abort the cherry-pick.

 In the case of a single cherry-pick, the distinction between
 skipping and aborting is not important, as there is no more
 work to be done afterwards.  When we are using the sequencer
 to cherry pick a series of commits, though, the instruction
 is confusing: does it skip this commit, or does it abort the
 rest of the cherry-pick?

 It does skip, after which the user can continue the
 cherry-pick. This is the right thing to be advising the user
 to do, but let's make it more clear what will happen, both
 by using the word skip, and by mentioning that the rest of
 the sequence can be continued via cherry-pick --continue
 (whether we skip or take the commit).

 Noticed-by: Ramkumar Ramachandra artag...@gmail.com
 Helped-by: Jonathan Nieder jrnie...@gmail.com
 Signed-off-by: Jeff King p...@peff.net
 ---
  builtin/commit.c | 25 ++---
  1 file changed, 22 insertions(+), 3 deletions(-)

 diff --git a/builtin/commit.c b/builtin/commit.c
 index e47f100..73fafe2 100644
 --- a/builtin/commit.c
 +++ b/builtin/commit.c
 @@ -63,8 +63,18 @@ N_(The previous cherry-pick is now empty, possibly due to 
 conflict resolution.\
  If you wish to commit it anyway, use:\n
  \n
  git commit --allow-empty\n
 +\n);
 +
 +static const char empty_cherry_pick_advice_single[] =
 +N_(Otherwise, please use 'git reset'\n);
 +
 +static const char empty_cherry_pick_advice_multi[] =
 +N_(If you wish to skip this commit, use:\n
  \n
 -Otherwise, please use 'git reset'\n);
 +git reset\n
 +\n
 +Then \git cherry-pick --continue\ will resume cherry-picking\n
 +the remaining commits.\n);
  
  static const char *use_message_buffer;
  static const char commit_editmsg[] = COMMIT_EDITMSG;
 @@ -107,6 +117,7 @@ static enum commit_whence whence;
  static const char *cleanup_arg;
  
  static enum commit_whence whence;
 +static int sequencer_in_use;
  static int use_editor = 1, include_status = 1;
  static int show_ignored_in_status, have_option_m;
  static const char *only_include_assumed;
 @@ -141,8 +152,11 @@ static void determine_whence(struct wt_status *s)
  {
   if (file_exists(git_path(MERGE_HEAD)))
   whence = FROM_MERGE;
 - else if (file_exists(git_path(CHERRY_PICK_HEAD)))
 + else if (file_exists(git_path(CHERRY_PICK_HEAD))) {
   whence = FROM_CHERRY_PICK;
 + if (file_exists(git_path(sequencer)))
 + sequencer_in_use = 1;

Should this be

sequencer_in_use = file_exists(...)

so readers do not have to wonder what the variable is initialized
to?

Other than that, looks reasonable to me.  Thanks.



 + }
   else
   whence = FROM_COMMIT;
   if (s)
 @@ -808,8 +822,13 @@ static int prepare_to_commit(const char *index_file, 
 const char *prefix,
   run_status(stdout, index_file, prefix, 0, s);
   if (amend)
   fputs(_(empty_amend_advice), stderr);
 - else if (whence == FROM_CHERRY_PICK)
 + else if (whence == FROM_CHERRY_PICK) {
   fputs(_(empty_cherry_pick_advice), stderr);
 + if (!sequencer_in_use)
 + fputs(_(empty_cherry_pick_advice_single), 
 stderr);
 + else
 + fputs(_(empty_cherry_pick_advice_multi), 
 stderr);
 + }
   return 0;
   }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-29 Thread Jeff King 
On Mon, Jul 29, 2013 at 08:18:45AM -0700, Junio C Hamano wrote:

  if (file_exists(git_path(MERGE_HEAD)))
  whence = FROM_MERGE;
  -   else if (file_exists(git_path(CHERRY_PICK_HEAD)))
  +   else if (file_exists(git_path(CHERRY_PICK_HEAD))) {
  whence = FROM_CHERRY_PICK;
  +   if (file_exists(git_path(sequencer)))
  +   sequencer_in_use = 1;
 
 Should this be
 
   sequencer_in_use = file_exists(...)
 
 so readers do not have to wonder what the variable is initialized
 to?

Yeah, I think that is a readability improvement. I suppose the use-site
could also just run file_exists itself, but I wanted to keep the
filesystem-reading magic name bits together.

I had also originally considered adding new states to whence to cover
these cases (i.e., FROM_CHERRY_PICK_MULTI), but there are fallouts all
over the place for call sites that do not care whether we are in the
single- or multi-commit mode.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-29 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 On Mon, Jul 29, 2013 at 08:18:45AM -0700, Junio C Hamano wrote:

 if (file_exists(git_path(MERGE_HEAD)))
 whence = FROM_MERGE;
  -  else if (file_exists(git_path(CHERRY_PICK_HEAD)))
  +  else if (file_exists(git_path(CHERRY_PICK_HEAD))) {
 whence = FROM_CHERRY_PICK;
  +  if (file_exists(git_path(sequencer)))
  +  sequencer_in_use = 1;
 
 Should this be
 
  sequencer_in_use = file_exists(...)
 
 so readers do not have to wonder what the variable is initialized
 to?

 Yeah, I think that is a readability improvement. I suppose the use-site
 could also just run file_exists itself, but I wanted to keep the
 filesystem-reading magic name bits together.

I take that one back.  The use-site is sufficiently far from this
assignment that is protected with a cascading if that the reader
needs to be aware that sequencer_in_use is initialized to zero
anyway.  The code you posted is just as readable, if not more.

 I had also originally considered adding new states to whence to cover
 these cases (i.e., FROM_CHERRY_PICK_MULTI), but there are fallouts all
 over the place for call sites that do not care whether we are in the
 single- or multi-commit mode.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-27 Thread Ramkumar Ramachandra 
Jeff King wrote:
  builtin/commit.c | 25 ++---
  1 file changed, 22 insertions(+), 3 deletions(-)

Overall, highly inelegant.  The single-commit pick has been special
cased only because we needed to preserve backward compatibility: I
would hate for the detail to be user-visible.  I'd have expected a
series that polishes sequencer.c instead.

But whatever.  I'm going to squelch them anyway.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-27 Thread Ramkumar Ramachandra 
Jonathan Nieder wrote:
 Your patch is just swapping out git reset for cherry-pick --abort,
 so I think that is a good improvement in the meantime.

 Um, wasn't the idea of the original message that you can run git
 reset and then git cherry-pick --continue?

No, and I don't know where you got that idea.  Certainly not by
reading the history.

37f7a85 (Teach commit about CHERRY_PICK_HEAD, 2011-02-19) introduced
that string.  This happened before I introduced cherry-pick --continue
in 5a5d80f (revert: Introduce --continue to continue the operation,
2011-08-04).

A proper solution to the problem would involve polishing sequencer.c,
and probably getting --skip-empty so it behaves more like rebase.  In
case you missed it, one person already did that work and posted the
code to the list [1].

[1]: http://article.gmane.org/gmane.comp.version-control.git/226982
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Ramkumar Ramachandra 
When a cherry-pick results in an empty commit, git prints:

  The previous cherry-pick is now empty, possibly due to conflict resolution.
  If you wish to commit it anyway, use:

  git commit --allow-empty

  Otherwise, please use 'git reset'

The last line is plain wrong in the case of a ranged pick, as a 'git
reset' will fail to remove $GIT_DIR/sequencer, failing a subsequent
cherry-pick or revert.  Change the advice to:

  git cherry-pick --abort

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
 Another candidate for maint?

 I'd also really like to squelch this with an advice.* variable; any
 suggestions?

 builtin/commit.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 003bd7d..1b213f7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -64,7 +64,10 @@ N_(The previous cherry-pick is now empty, possibly due to 
conflict resolution.\
 \n
 git commit --allow-empty\n
 \n
-Otherwise, please use 'git reset'\n);
+Otherwise, use:\n
+\n
+git cherry-pick --abort\n
+\n);
 
 static const char *use_message_buffer;
 static const char commit_editmsg[] = COMMIT_EDITMSG;
-- 
1.8.4.rc0.1.g8f6a3e5.dirty

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Jeff King 
On Fri, Jul 26, 2013 at 11:42:00PM +0530, Ramkumar Ramachandra wrote:

 When a cherry-pick results in an empty commit, git prints:
 
   The previous cherry-pick is now empty, possibly due to conflict resolution.
   If you wish to commit it anyway, use:
 
   git commit --allow-empty
 
   Otherwise, please use 'git reset'
 
 The last line is plain wrong in the case of a ranged pick, as a 'git
 reset' will fail to remove $GIT_DIR/sequencer, failing a subsequent
 cherry-pick or revert.  Change the advice to:
 
   git cherry-pick --abort

Hmm. I don't think I've run across this message myself, so perhaps I do
not understand the situation. But it seems like you would want to do one
of:

  1. Make an empty commit.

  2. Skip this commit and continue the rest of the cherry-pick sequence.

  3. Abort the cherry pick sequence.

Those are the options presented when rebase runs into an empty commit,
where (2) is presented as rebase --skip. I'm not sure how to do that
here; is it just cherry-pick --continue?

  I'd also really like to squelch this with an advice.* variable; any
  suggestions?

This seems like a good candidate for squelching, but you would probably
want to split it. The two parts of the message are:

  1. What happened (the cherry-pick is empty).

  2. How to proceed from here (allow-empty, abort, etc).

You still want to say (1), but (2) is useless to old-timers.  Probably
something like advice.cherryPickInstructions would be a good name for an
option to squelch (2), and it should apply wherever we tell the user how
to proceed. Potentially it should even be advice.sequenceInstructions,
and apply to rebase and am as well.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Ramkumar Ramachandra 
Jeff King wrote:
 Hmm. I don't think I've run across this message myself, so perhaps I do
 not understand the situation.

It's very simple.

  # on branch master
  $ git checkout -b test
  $ git cherry-pick master
  $ ls .git/sequencer
  # missing

In the pseudo multi-pick case (I say pseudo because there's really
just one commit to pick):

  # on branch master
  $ git checkout -b test
  $ git cherry-pick master~..
  $ ls .git/sequencer

cat .git/sequencer/todo if you like.

   2. Skip this commit and continue the rest of the cherry-pick sequence.

Nope, this is unsupported afaik.

 Those are the options presented when rebase runs into an empty commit,
 where (2) is presented as rebase --skip. I'm not sure how to do that
 here; is it just cherry-pick --continue?

No, --continue will just print the same message over and over again.

Yes, the whole ranged cherry-pick thing can use a lot more polish.

   1. What happened (the cherry-pick is empty).

   2. How to proceed from here (allow-empty, abort, etc).

 You still want to say (1), but (2) is useless to old-timers.  Probably
 something like advice.cherryPickInstructions would be a good name for an
 option to squelch (2), and it should apply wherever we tell the user how
 to proceed. Potentially it should even be advice.sequenceInstructions,
 and apply to rebase and am as well.

Good suggestion.  I'll pick advice.cherryPickInstructions when I
decide to polish sequencer.c a bit.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Jeff King 
On Sat, Jul 27, 2013 at 02:47:47AM +0530, Ramkumar Ramachandra wrote:

2. Skip this commit and continue the rest of the cherry-pick sequence.
 
 Nope, this is unsupported afaik.

Ah. I don't mind improving the message in the meantime, but it sounds
like this is a deficiency in sequenced cherry-pick that needs addressed
eventually.

Your patch is just swapping out git reset for cherry-pick --abort,
so I think that is a good improvement in the meantime.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Ramkumar Ramachandra 
Jeff King wrote:
 Ah. I don't mind improving the message in the meantime, but it sounds
 like this is a deficiency in sequenced cherry-pick that needs addressed
 eventually.

I'm especially irked by how slow rebase--am is, and want to replace it.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Jonathan Nieder 
Jeff King wrote:

 Your patch is just swapping out git reset for cherry-pick --abort,
 so I think that is a good improvement in the meantime.

Um, wasn't the idea of the original message that you can run git
reset and then git cherry-pick --continue?

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Jeff King 
On Fri, Jul 26, 2013 at 02:37:05PM -0700, Jonathan Nieder wrote:

 Jeff King wrote:
 
  Your patch is just swapping out git reset for cherry-pick --abort,
  so I think that is a good improvement in the meantime.
 
 Um, wasn't the idea of the original message that you can run git
 reset and then git cherry-pick --continue?

Maybe. :)

I missed that subtlety. Of my three things you would want to do, that
means it was _trying_ say number 2, how to skip, rather than 3, how to
abort. If that is the case, then it should probably explain the sequence
of steps as reset and then --continue to make it more clear.

I.e., a patch is needed, but Ram's is going in the opposite direction.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Jeff King 
On Fri, Jul 26, 2013 at 05:40:36PM -0400, Jeff King wrote:

  Jeff King wrote:
  
   Your patch is just swapping out git reset for cherry-pick --abort,
   so I think that is a good improvement in the meantime.
  
  Um, wasn't the idea of the original message that you can run git
  reset and then git cherry-pick --continue?
 
 Maybe. :)
 
 I missed that subtlety. Of my three things you would want to do, that
 means it was _trying_ say number 2, how to skip, rather than 3, how to
 abort. If that is the case, then it should probably explain the sequence
 of steps as reset and then --continue to make it more clear.
 
 I.e., a patch is needed, but Ram's is going in the opposite direction.

I played around a bit with the test cases that Ram showed. It seems like
the advice needed is different depending on whether you are in a single
or multi-commit cherry-pick.

So if we hit an empty commit and you want to:

  1. Make an empty commit, then always run git commit --allow-empty.

  2. Skip this commit, then if:

 a. this is a single commit cherry-pick, you run git reset (and
nothing more, the cherry pick is finished; running cherry-pick
--continue will yield an error).

 b. this is a multi-commit cherry-pick, you run git reset,
followed by git cherry-pick --continue

  3. Abort the commit, run git cherry-pick --abort

Let's assume that the instructions we want to give the user are how to
do options 1 and 2. I do not mind omitting 3, as it should be reasonably
obvious that cherry-pick --abort is always good way to abort.

So we give good instructions for the single-commit case, but bad
instructions for the multi-commit case. Ram's patch suggests --abort
instead of reset, which is the same for the single-commit case, but
suggests 3 instead of 2 for the multi-patch.

I think instead we would want to leave the single-commit case alone, and
for the multi-commit case add ...and then cherry-pick --continue. That
message is generated from within git-commit, though; I guess it would
need to learn about the difference between single/multi cherry-picks.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Jeff King 
On Fri, Jul 26, 2013 at 06:43:59PM -0400, Jeff King wrote:

 I think instead we would want to leave the single-commit case alone, and
 for the multi-commit case add ...and then cherry-pick --continue. That
 message is generated from within git-commit, though; I guess it would
 need to learn about the difference between single/multi cherry-picks.

Like this?

-- 8 --
Subject: [PATCH] commit: tweak empty cherry pick advice for sequencer

When we refuse to make an empty commit, we check whether we
are in a cherry-pick in order to give better advice on how
to proceed. We instruct the user to repeat the commit with
--allow-empty to force the commit, or to use git reset
to skip it and abort the cherry-pick.

This works fine when we are cherry-picking a single commit.
When we are using the sequencer to cherry-pick multiple
items, though, using git reset is not good advice. It does
not skip the commit (you must run cherry-pick --continue
to keep going), but nor does it abort the cherry-pick (the
.sequencer directory still exists).

Let's teach commit to tell when the sequencer is in use, and
to mention cherry-pick --continue in that case.

Signed-off-by: Jeff King p...@peff.net
---
The advice in the multi case could eventually change to cherry-pick
--skip if and when it exists.

 builtin/commit.c | 23 ---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e47f100..45a98d7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -63,8 +63,14 @@ N_(The previous cherry-pick is now empty, possibly due to 
conflict resolution.\
 If you wish to commit it anyway, use:\n
 \n
 git commit --allow-empty\n
+\n);
+static const char empty_cherry_pick_advice_skip_single[] =
+N_(Otherwise, please use 'git reset'\n);
+static const char empty_cherry_pick_advice_skip_multi[] =
+N_(If you wish to skip this commit, use:\n
 \n
-Otherwise, please use 'git reset'\n);
+git reset  git cherry-pick --continue\n
+\n);
 
 static const char *use_message_buffer;
 static const char commit_editmsg[] = COMMIT_EDITMSG;
@@ -107,6 +113,7 @@ static enum commit_whence whence;
 static const char *cleanup_arg;
 
 static enum commit_whence whence;
+static int sequencer_in_use;
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status, have_option_m;
 static const char *only_include_assumed;
@@ -141,8 +148,11 @@ static void determine_whence(struct wt_status *s)
 {
if (file_exists(git_path(MERGE_HEAD)))
whence = FROM_MERGE;
-   else if (file_exists(git_path(CHERRY_PICK_HEAD)))
+   else if (file_exists(git_path(CHERRY_PICK_HEAD))) {
whence = FROM_CHERRY_PICK;
+   if (file_exists(git_path(sequencer)))
+   sequencer_in_use = 1;
+   }
else
whence = FROM_COMMIT;
if (s)
@@ -808,8 +818,15 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
run_status(stdout, index_file, prefix, 0, s);
if (amend)
fputs(_(empty_amend_advice), stderr);
-   else if (whence == FROM_CHERRY_PICK)
+   else if (whence == FROM_CHERRY_PICK) {
fputs(_(empty_cherry_pick_advice), stderr);
+   if (!sequencer_in_use)
+   fputs(_(empty_cherry_pick_advice_skip_single),
+ stderr);
+   else
+   fputs(_(empty_cherry_pick_advice_skip_multi),
+ stderr);
+   }
return 0;
}
 
-- 
1.8.3.rc1.30.gff0fb75

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Jonathan Nieder 
Jeff King wrote:

 --- a/builtin/commit.c
 +++ b/builtin/commit.c
 @@ -63,8 +63,14 @@ N_(The previous cherry-pick is now empty, possibly due to 
 conflict resolution.\
  If you wish to commit it anyway, use:\n
  \n
  git commit --allow-empty\n
 +\n);
 +static const char empty_cherry_pick_advice_skip_single[] =
 +N_(Otherwise, please use 'git reset'\n);
 +static const char empty_cherry_pick_advice_skip_multi[] =
 +N_(If you wish to skip this commit, use:\n
  \n
 -Otherwise, please use 'git reset'\n);
 +git reset  git cherry-pick --continue\n
 +\n);

Hmm, wouldn't it be more consistent to either say

If you wish to commit it anyway, use

git commit --allow-empty  git cherry-pick --continue

If you wish to skip this commit, use

git reset  git cherry-pick --continue

Or

If you wish to commit it anyway, use

git commit --allow-empty

If you wish to skip this commit, use

git reset

Then git cherry-pick --continue will resume cherry-picking
the remaining commits.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Jeff King 
On Fri, Jul 26, 2013 at 04:08:57PM -0700, Jonathan Nieder wrote:

 Jeff King wrote:
 
  --- a/builtin/commit.c
  +++ b/builtin/commit.c
  @@ -63,8 +63,14 @@ N_(The previous cherry-pick is now empty, possibly due 
  to conflict resolution.\
   If you wish to commit it anyway, use:\n
   \n
   git commit --allow-empty\n
  +\n);
  +static const char empty_cherry_pick_advice_skip_single[] =
  +N_(Otherwise, please use 'git reset'\n);
  +static const char empty_cherry_pick_advice_skip_multi[] =
  +N_(If you wish to skip this commit, use:\n
   \n
  -Otherwise, please use 'git reset'\n);
  +git reset  git cherry-pick --continue\n
  +\n);
 
 Hmm, wouldn't it be more consistent to either say
 
   If you wish to commit it anyway, use
 
   git commit --allow-empty  git cherry-pick --continue
 
   If you wish to skip this commit, use
 
   git reset  git cherry-pick --continue

Good point. Clearly the original assumed that you knew to cherry-pick
--continue, since it is needed (and omitted) in both cases. And perhaps
most people do, but certainly the lack of mentioning it confused both me
and Ram about whether the git reset advice was meant to skip or abort.

 Or
 
   If you wish to commit it anyway, use
 
   git commit --allow-empty
   
   If you wish to skip this commit, use
 
   git reset
 
   Then git cherry-pick --continue will resume cherry-picking
   the remaining commits.

I like this one better.

You could _almost_ just use the top bit for the single-commit case, but
I hesitate to use the word skip in that case. Right now the
single-commit case does not need to make the distinction between skip
this, and there is nothing else to do and abort the operation,
because they are the same thing.  Whichever way the user thinks about it
is OK.

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

Re: [PATCH] commit: correct advice about aborting a cherry-pick

2013-07-26 Thread Jeff King 
On Fri, Jul 26, 2013 at 07:19:02PM -0400, Jeff King wrote:

  Or
  
  If you wish to commit it anyway, use
  
  git commit --allow-empty
  
  If you wish to skip this commit, use
  
  git reset
  
  Then git cherry-pick --continue will resume cherry-picking
  the remaining commits.
 
 I like this one better.

Here it is in patch form, with an updated commit message that hopefully
explains the rationale a bit better.

-- 8 --
Subject: [PATCH] commit: tweak empty cherry pick advice for sequencer

When we refuse to make an empty commit, we check whether we
are in a cherry-pick in order to give better advice on how
to proceed. We instruct the user to repeat the commit with
--allow-empty to force the commit, or to use git reset
to skip it and abort the cherry-pick.

In the case of a single cherry-pick, the distinction between
skipping and aborting is not important, as there is no more
work to be done afterwards.  When we are using the sequencer
to cherry pick a series of commits, though, the instruction
is confusing: does it skip this commit, or does it abort the
rest of the cherry-pick?

It does skip, after which the user can continue the
cherry-pick. This is the right thing to be advising the user
to do, but let's make it more clear what will happen, both
by using the word skip, and by mentioning that the rest of
the sequence can be continued via cherry-pick --continue
(whether we skip or take the commit).

Noticed-by: Ramkumar Ramachandra artag...@gmail.com
Helped-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Jeff King p...@peff.net
---
 builtin/commit.c | 25 ++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e47f100..73fafe2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -63,8 +63,18 @@ N_(The previous cherry-pick is now empty, possibly due to 
conflict resolution.\
 If you wish to commit it anyway, use:\n
 \n
 git commit --allow-empty\n
+\n);
+
+static const char empty_cherry_pick_advice_single[] =
+N_(Otherwise, please use 'git reset'\n);
+
+static const char empty_cherry_pick_advice_multi[] =
+N_(If you wish to skip this commit, use:\n
 \n
-Otherwise, please use 'git reset'\n);
+git reset\n
+\n
+Then \git cherry-pick --continue\ will resume cherry-picking\n
+the remaining commits.\n);
 
 static const char *use_message_buffer;
 static const char commit_editmsg[] = COMMIT_EDITMSG;
@@ -107,6 +117,7 @@ static enum commit_whence whence;
 static const char *cleanup_arg;
 
 static enum commit_whence whence;
+static int sequencer_in_use;
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status, have_option_m;
 static const char *only_include_assumed;
@@ -141,8 +152,11 @@ static void determine_whence(struct wt_status *s)
 {
if (file_exists(git_path(MERGE_HEAD)))
whence = FROM_MERGE;
-   else if (file_exists(git_path(CHERRY_PICK_HEAD)))
+   else if (file_exists(git_path(CHERRY_PICK_HEAD))) {
whence = FROM_CHERRY_PICK;
+   if (file_exists(git_path(sequencer)))
+   sequencer_in_use = 1;
+   }
else
whence = FROM_COMMIT;
if (s)
@@ -808,8 +822,13 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
run_status(stdout, index_file, prefix, 0, s);
if (amend)
fputs(_(empty_amend_advice), stderr);
-   else if (whence == FROM_CHERRY_PICK)
+   else if (whence == FROM_CHERRY_PICK) {
fputs(_(empty_cherry_pick_advice), stderr);
+   if (!sequencer_in_use)
+   fputs(_(empty_cherry_pick_advice_single), 
stderr);
+   else
+   fputs(_(empty_cherry_pick_advice_multi), 
stderr);
+   }
return 0;
}
 
-- 
1.8.3.rc1.30.gff0fb75

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