| Message ID | 20190608191958.4593-2-rohit.ashiwal265@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Teach cherry-pick/revert to skip commits | expand |
Hi Rohit Congratulations on your first GSoC patch series! On 08/06/2019 20:19, Rohit Ashiwal wrote: > In the case of merge conflicts, while performing a revert, we are > currently advised to use `git cherry-pick --<sequencer-options>` > of which --continue is incompatible for continuing the revert. > Introduce a separate advice message for `git revert`. > > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > sequencer.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/sequencer.c b/sequencer.c > index f88a97fb10..9c561a041b 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2655,6 +2655,7 @@ static int create_seq_dir(void) > if (file_exists(git_path_seq_dir())) { > error(_("a cherry-pick or revert is already in progress")); > advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); > + advise(_("or \"git revert (--continue | --quit | --abort)\"")); I agree that it's a good idea to add advice for revert as well, but it would be better to call sequencer_get_last_command() to find out if we're running cherry-pick or revert and tailor the advice appropriately. Best Wishes Phillip > return -1; > } else if (mkdir(git_path_seq_dir(), 0777) < 0) > return error_errno(_("could not create sequencer directory '%s'"), >
Hi Philip On 2019-06-09 17:52 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Rohit > > Congratulations on your first GSoC patch series! Thank you very much :) > On 08/06/2019 20:19, Rohit Ashiwal wrote: > > [...] > > @@ -2655,6 +2655,7 @@ static int create_seq_dir(void) > > if (file_exists(git_path_seq_dir())) { > > error(_("a cherry-pick or revert is already in progress")); > > advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); > > + advise(_("or \"git revert (--continue | --quit | --abort)\"")); > > I agree that it's a good idea to add advice for revert as well, but it > would be better to call sequencer_get_last_command() to find out if > we're running cherry-pick or revert and tailor the advice appropriately. Firstly, signature of `create_seq_dir` doesn't allow us to call `sequencer_get_last_command()`. Changing that for the sake of a better error message is too much task for this patch as it is a subject of discussion on its own. (Also changing signature only makes sense if this patch series gets merged). FWIW, I think we should left this to further discussions for now and decide what to do later on. > Best Wishes > > Phillip Thanks Rohit
Hi Rohit On 10/06/2019 06:13, Rohit Ashiwal wrote: > Hi Philip > > On 2019-06-09 17:52 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> Hi Rohit >> >> Congratulations on your first GSoC patch series! > > Thank you very much :) > >> On 08/06/2019 20:19, Rohit Ashiwal wrote: >>> [...] >>> @@ -2655,6 +2655,7 @@ static int create_seq_dir(void) >>> if (file_exists(git_path_seq_dir())) { >>> error(_("a cherry-pick or revert is already in progress")); >>> advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); >>> + advise(_("or \"git revert (--continue | --quit | --abort)\"")); >> >> I agree that it's a good idea to add advice for revert as well, but it >> would be better to call sequencer_get_last_command() to find out if >> we're running cherry-pick or revert and tailor the advice appropriately. > > Firstly, signature of `create_seq_dir` doesn't allow us to call > `sequencer_get_last_command()`. Changing that for the sake of a > better error message is too much task for this patch as it is a > subject of discussion on its own. There is only one caller and it already has a struct repository pointer so it is a two line change, one of which is the insertion of a single character to change create_seq_dir() so it can call sequencer_get_last_command(). It is normal to change function signatures (especially for static functions like this) when making changes, it is part of improving the code base. The quality of error messages is important to the overall user experience. It's when things go wrong that users need accurate advice about what to do. > (Also changing signature only > makes sense if this patch series gets merged). FWIW, I think we > should left this to further discussions for now and decide what > to do later on. It is only a small change so why not do it now rather than putting it off for another series which will be more work in the long run. Best Wishes Phillip > >> Best Wishes >> >> Phillip > > Thanks > Rohit >
Hi Phillip On 2019-06-10 10:39 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: > >Hi Rohit > >On 10/06/2019 06:13, Rohit Ashiwal wrote: >> >> [...] >> Firstly, signature of `create_seq_dir` doesn't allow us to call >> `sequencer_get_last_command()`. Changing that for the sake of a >> better error message is too much task for this patch as it is a >> subject of discussion on its own. > > There is only one caller and it already has a struct repository pointer > so it is a two line change, one of which is the insertion of a single > character to change create_seq_dir() so it can call > sequencer_get_last_command(). It is normal to change function signatures > (especially for static functions like this) when making changes, it is > part of improving the code base. The quality of error messages is > important to the overall user experience. It's when things go wrong that > users need accurate advice about what to do. Nice! I'll do this in next revision then. >> (Also changing signature only >> makes sense if this patch series gets merged). FWIW, I think we >> should left this to further discussions for now and decide what >> to do later on. > > It is only a small change so why not do it now rather than putting it > off for another series which will be more work in the long run. I'm fine with changing that here. Thanks Rohit
Phillip Wood <phillip.wood123@gmail.com> writes: >> Firstly, signature of `create_seq_dir` doesn't allow us to call >> `sequencer_get_last_command()`. Changing that for the sake of a >> better error message is too much task for this patch as it is a >> subject of discussion on its own. > > There is only one caller and it already has a struct repository pointer > so it is a two line change, one of which is the insertion of a single > character to change create_seq_dir() so it can call > sequencer_get_last_command(). It is normal to change function signatures > (especially for static functions like this) when making changes, it is > part of improving the code base. The quality of error messages is > important to the overall user experience. It's when things go wrong that > users need accurate advice about what to do. Thanks for guiding a new contributor in the right direction gently. Very much appreciated.
Hi Rohit On 10/06/2019 14:25, Rohit Ashiwal wrote: > Hi Phillip > > On 2019-06-10 10:39 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> Hi Rohit >> >> On 10/06/2019 06:13, Rohit Ashiwal wrote: >>> >>> [...] >>> Firstly, signature of `create_seq_dir` doesn't allow us to call >>> `sequencer_get_last_command()`. Changing that for the sake of a >>> better error message is too much task for this patch as it is a >>> subject of discussion on its own. >> >> There is only one caller and it already has a struct repository pointer >> so it is a two line change, one of which is the insertion of a single >> character to change create_seq_dir() so it can call >> sequencer_get_last_command(). It is normal to change function signatures >> (especially for static functions like this) when making changes, it is >> part of improving the code base. The quality of error messages is >> important to the overall user experience. It's when things go wrong that >> users need accurate advice about what to do. > > Nice! I'll do this in next revision then. I'm glad that helped > >>> (Also changing signature only >>> makes sense if this patch series gets merged). FWIW, I think we >>> should left this to further discussions for now and decide what >>> to do later on. >> >> It is only a small change so why not do it now rather than putting it >> off for another series which will be more work in the long run. > > I'm fine with changing that here. That's great Best Wishes Phillip > > Thanks > Rohit >
diff --git a/sequencer.c b/sequencer.c index f88a97fb10..9c561a041b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2655,6 +2655,7 @@ static int create_seq_dir(void) if (file_exists(git_path_seq_dir())) { error(_("a cherry-pick or revert is already in progress")); advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); + advise(_("or \"git revert (--continue | --quit | --abort)\"")); return -1; } else if (mkdir(git_path_seq_dir(), 0777) < 0) return error_errno(_("could not create sequencer directory '%s'"),
In the case of merge conflicts, while performing a revert, we are currently advised to use `git cherry-pick --<sequencer-options>` of which --continue is incompatible for continuing the revert. Introduce a separate advice message for `git revert`. Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- sequencer.c | 1 + 1 file changed, 1 insertion(+)