From: Mehul Jain Date: Fri, 11 Mar 2016 13:15:01 GMT Subject: Re: [PATCH v7 2/2] pull --rebase: add --[no-]autostash flag Message-ID: In-Reply-To: On Fri, Mar 11, 2016 at 10:21 AM, Paul Tan wrote: >> static int config_autostash = -1; > > Hmm, why can't config_autostash just default to 0? Previously Junio recommended not to explicitly initialize a static to 0 (or NULL). http://thread.gmane.org/gmane.comp.version-control.git/287709/focus=287726 Defaulting config_autostash = 0 will work too as you explained. >> + if (opt_autostash == 1) >> + argv_array_push(&args, "--autostash"); >> + else if (opt_autostash == 0) >> + argv_array_push(&args, "--no-autostash"); > > The precise testing for specific values of -1, 0 and 1 throughout the > code makes me uncomfortable. Ordinarily, I would expect a simple > > argv_array_push(&args, opt_autostash ? "--autostash" : "--no-autostash"); This looks good. I will change accordingly. > Stepping back a bit, the only reason why we introduced opt_autostash = > -1 as a possible value is because we need to detect if > --[no-]autostash is being used with git-merge. I consider that a > kludge, because if git-merge supports --autostash as well (possibly in > the future), then we will not need this -1 value. > > So, from that point of view, a -1 value is okay as a workaround, but > kludges, and hence the -1 value, should be gotten rid off as soon as > possible. That right! But until git-merge doesn't support --autostash, it's necessary to have opt_autostash = -1 as default. I wonder if it will be a good thing to add the following line to the commit message "Changes needed to be introduced: Change opt_autostash = 0 as default as soon as git-merge supports --autostash option, also display no error when "git pull --autostash" is called." Possibly it would be better to add gmane link of your review in the commit message for further clarification. Thanks, Mehul