"git am" and then "git am -3" regression?
Hmm, there seems to be some glitches around running "am -3" after a failed "am" between 'maint' and 'master'. When I try the following sequence, the 'am' from 'maint' succeeds, but 'am' in 'master' fails: * Save Eric's "minor documetation improvements" $gmane/274537 to a file. * "git checkout e177995" (that's "next^0") and then apply them with "git am" (no -3 necessary). * "git checkout 272be14" (that's "es/worktree-add-cleanup^0") and then apply them with "git am" (without -3). This is expected to stop at 2/6, as the context has changed between 272be14 and the tip of 'next'. * "git am -3". This should restart and resolve cleanly. Reverting d96a275b91bae1800cd43be0651e886e7e042a17 seems to fix it, so that is what I'll do for 2.5 final. I think Paul's builtin-am has the same issues, that would need a separate fix. commit d96a275b91bae1800cd43be0651e886e7e042a17 Author: Remi Lespinet Date: Thu Jun 4 17:04:55 2015 +0200 git-am: add am.threeWay config variable Add the am.threeWay configuration variable to use the -3 or --3way option of git am by default. When am.threeway is set and not desired for a specific git am command, the --no-3way option can be used to override 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: "git am" and then "git am -3" regression?
On Fri, Jul 24, 2015 at 10:48:18AM -0700, Junio C Hamano wrote: > Hmm, there seems to be some glitches around running "am -3" > after a failed "am" between 'maint' and 'master'. > > When I try the following sequence, the 'am' from 'maint' succeeds, > but 'am' in 'master' fails: > > * Save Eric's "minor documetation improvements" $gmane/274537 >to a file. > > * "git checkout e177995" (that's "next^0") and then apply them with >"git am" (no -3 necessary). > > * "git checkout 272be14" (that's "es/worktree-add-cleanup^0") and >then apply them with "git am" (without -3). > >This is expected to stop at 2/6, as the context has changed >between 272be14 and the tip of 'next'. > > * "git am -3". This should restart and resolve cleanly. Thanks for diagnosing. This bit me the other day, but I hadn't had time to look at it yet (and I "am" a lot less than you do, I imagine). > Reverting d96a275b91bae1800cd43be0651e886e7e042a17 seems to fix it, > so that is what I'll do for 2.5 final. Yeah, I think this hunk is to blame (though I just read the code and did not test): @@ -658,6 +665,8 @@ fi if test "$(cat "$dotest/threeway")" = t then threeway=t +else + threeway=f fi It comes after the command-line option parsing, so it overrides our option (I think that running "git am -3" followed by "git am --no-3way" would have the same problem). It cannot just check whether $threeway is unset, though, as it may have come from the config. We'd need a separate variable, the way the code is ordered now. Ideally the code would just be ordered as: - load config from git-config - override that with defaults inherited from a previous run - override that with command-line parsing but I don't know if there are other ordering gotchas that would break. It does look like that is how Paul's builtin/am.c does it, which makes me think it might not be broken. It's also possibly I've horribly misdiagnosed the bug. ;) -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: "git am" and then "git am -3" regression?
On Sat, Jul 25, 2015 at 2:09 AM, Jeff King wrote: > Yeah, I think this hunk is to blame (though I just read the code and did not > test): > > @@ -658,6 +665,8 @@ fi > if test "$(cat "$dotest/threeway")" = t > then > threeway=t > +else > + threeway=f > fi > > It comes after the command-line option parsing, so it overrides our option (I > think that running "git am -3" followed by "git am --no-3way" would have the > same problem). It cannot just check whether $threeway is unset, though, as it > may have come from the config. Thanks for the detailed analysis, I completely agree. Note that the code that handles the --message-id option somewhat handles the case where $messageid is unset: case "$(cat "$dotest/messageid")" in t) messageid=-m ;; f) messageid= ;; esac However, it still does not handle "git am --no-message-id" followed by "git am --message-id", or "git -c am.messageid=true am" followed by "git am --no-message-id". I think the same thing occurs for --scissors/--no-scissors, as well as the git-apply options as well. The real problem is that the state directory loading code comes after the config loading and option parsing code, and thus overrides any variables set. > We'd need a separate variable, the way the code > is ordered now. If we are just fixing --3way, adding one extra variable won't be that bad. However, I think that if we are using this approach to fix all of the options, then it would introduce too much code complexity. > Ideally the code would just be ordered as: > > - load config from git-config > > - override that with defaults inherited from a previous run > > - override that with command-line parsing So I'm more in favor of this solution. It's feels much more natural to me, rather than attempting to workaround the existing code structure. > but I don't know if there are other ordering gotchas that would break. For the C code, there won't be any problem, but yeah, fixing it in git-am.sh might need a bit more effort. > It does look like that is how Paul's builtin/am.c does it, which makes > me think it might not be broken. It's also possibly I've horribly > misdiagnosed the bug. ;) Nah, it follows the same structure as git-am.sh and so will exhibit the same behavior. It currently does something like this: 1. am_state_init() (config settings are loaded) 2. parse_options() 3. if (am_in_progress()) am_load(); else am_setup(); So it would be quite trivial to change the control flow such that it is: 1. am_state_init() 2. if (am_in_progress()) am_load() 3. parse_options(); 4 if (!am_in_progress()) am_setup() The next question is, should any options set on the command-line affect subsequent invocations? If yes, then the control flow will be like: 1. am_state_init(); 2. if (am_in_progress()) am_load(); 3. parse_options(); 4. if (am_in_progress()) am_save_opts(); else am_setup(); where am_save_opts() will write the updated variables back to the state directory. What do you think? Since the builtin-am series is in 'next' already, and the fix in C is straightforward, to save time and effort I'm wondering if we could just do "am.threeWay patch -> builtin-am series -> bugfix patch in C". My university term is starting soon so I may not have so much time, but I'll see what I can do :-/ Junio, how do you want to proceed? Thanks, Paul -- 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: "git am" and then "git am -3" regression?
On Sun, Jul 26, 2015 at 01:03:59PM +0800, Paul Tan wrote: > > Ideally the code would just be ordered as: > > > > - load config from git-config > > > > - override that with defaults inherited from a previous run > > > > - override that with command-line parsing > > So I'm more in favor of this solution. It's feels much more natural to > me, rather than attempting to workaround the existing code structure. Yeah, I really prefer it, too. I just didn't know if there would be other confusing fallouts from changing the ordering. But since you have been deep in this code recently, I trust your judgement. :) > > It does look like that is how Paul's builtin/am.c does it, which makes > > me think it might not be broken. It's also possibly I've horribly > > misdiagnosed the bug. ;) > > Nah, it follows the same structure as git-am.sh and so will exhibit > the same behavior. It currently does something like this: > > 1. am_state_init() (config settings are loaded) > 2. parse_options() > 3. if (am_in_progress()) am_load(); else am_setup(); Ah, right. I took the am_state_init() to be the part where we loaded the existing options, and didn't notice the later am_load(). > The next question is, should any options set on the command-line > affect subsequent invocations? If yes, then the control flow will be > like: > > 1. am_state_init(); > 2. if (am_in_progress()) am_load(); > 3. parse_options(); > 4. if (am_in_progress()) am_save_opts(); else am_setup(); > > where am_save_opts() will write the updated variables back to the > state directory. What do you think? I don't think we need to go that direction. The usual thought process (mine, anyway) is: 1. I want to apply a series, and I want to use option A. 2. Oops, one of the patches didn't apply. Let's retry it with option B (usually "-3"). 3. OK, that worked. Now let's try the rest of the patches. I wouldn't expect in step 3 to have options from step 2 persist. That was just about wiggling that _one_ patch. Whereas options from step 1 are about the whole series. > Since the builtin-am series is in 'next' already, and the fix in C is > straightforward, to save time and effort I'm wondering if we could > just do "am.threeWay patch -> builtin-am series -> bugfix patch in C". > My university term is starting soon so I may not have so much time, > but I'll see what I can do :-/ Yeah, having to worry about two implementations of "git am" is a real pain. If we are close on merging the builtin version, it makes sense to me to hold off on the am.threeway feature until that is merged. Trying to fix the ordering of the script that is going away isn't a good use of anybody's time. -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: "git am" and then "git am -3" regression?
Jeff King writes: > Yeah, having to worry about two implementations of "git am" is a real > pain. If we are close on merging the builtin version, it makes sense to > me to hold off on the am.threeway feature until that is merged. Trying > to fix the ordering of the script that is going away isn't a good use of > anybody's time. So, the best option seems to be: 1) Revert d96a275 (git-am: add am.threeWay config variable, 2015-06-04) 2) Include the C port of d96a275 together with tests and docs verbatim from d96a275 into Paul's series. Actually, doing 1) is probably a good idea anyway, there's no reason to hold the release for such minor feature. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: "git am" and then "git am -3" regression?
On Mon, Jul 27, 2015 at 10:09:43AM +0200, Matthieu Moy wrote: > > Yeah, having to worry about two implementations of "git am" is a real > > pain. If we are close on merging the builtin version, it makes sense to > > me to hold off on the am.threeway feature until that is merged. Trying > > to fix the ordering of the script that is going away isn't a good use of > > anybody's time. > > So, the best option seems to be: > > 1) Revert d96a275 (git-am: add am.threeWay config variable, 2015-06-04) > > 2) Include the C port of d96a275 together with tests and docs verbatim >from d96a275 into Paul's series. > > Actually, doing 1) is probably a good idea anyway, there's no reason to > hold the release for such minor feature. I think step 1 is done already for v2.5.0, in 15dc5b5. We _could_ fix it in the script version for the upcoming cycle, but if we are merging builtin-am during this cycle, I do not see the point. -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: "git am" and then "git am -3" regression?
Paul Tan writes: > Junio, how do you want to proceed? I'd expect that builtin series would graduate in 2 releases from now at the latest, if not earlier. Let's just revert the regressing change from the scripted version and have it implemented in the builtin one in the meantime. I do not think it is worth adding features to the scripted one at this point. 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
