Akos Vandra wrote: > Thank you for the advice, I will do my best to live up to them :)
Cool. > I'm new to git as well, I tried making the patch you requested: Some feedback. First, please see if you can send patches as text/plain attachments (optionally with a Content-Disposition: inline MIME header) because when they are included in the body of the message usually they will be destroyed somehow by either the sending or the receiving email software. :\ > From 88a430100be4d9685863aa2912173ddc5b83e92f Mon Sep 17 00:00:00 2001 > From: Akos <akos@FM12BQ.(none)> Please fix this to look nice by running: git config --global user.name 'Akos Vandra' git config --global user.email '[email protected]' And then please fix the authorship information for this commit. You will now already jump into one of the most powerful features of git; rewriting history. Recommend reading Pro Git on this topic: http://progit.org/book/ch6-4.html So if this is the last commit on the branch that you are working with then run git commit --amend --author='Akos Vandra <[email protected]>' and save the commit message. NB I think you need a not too old version of git for --author to work. It's possible that what is in Ubuntu 10.10 is too old. :\ In that case you can still do it, but it gets a bit more complicated. You need to say --author because git tracks authorship separate from who committed. Run git log --pretty=fuller to see both fields. The user.(name|email) settings will only be used for Commit information when doing git commit --amend, because even though you are making a new commit it does not mean that you are now suddenly the author of this change, so you have to manually set a new Author on the command line. > Date: Sun, 25 Sep 2011 23:55:36 +0200 > Subject: [PATCH] Fixed two warnings for uninitialized variables > generating build errors > > Two retvals in the functions oocd_swd_transport_init and > mem_ap_read_buf_u32 have not had their values initialized > and generated a warning, which cause the build to fail. > Fixed by initializing these variables to ERROR_OK Why was ERROR_OK in particular chosen? Is it the common pattern in other functions? Or did you check these two functions in detail to discover that the unused variable will be used in the success case? It is a good idea to not neccessarily focus on *what* is changed (when this can trivially be learned from the patch itself) but more importantly discuss *why* the change was made in the particular way it was. > +++ b/src/target/arm_adi_v5.c > @@ -696,7 +696,7 @@ int mem_ap_read_buf_u32(struct adiv5_dap *dap, > uint8_t *buffer, > // uint32_t invalue, adr = address; > // uint8_t* pBuffer = buffer; > > - int i, retval; > + int i, retval = ERROR_OK; .. > +++ b/src/transport/swd_core.c > @@ -125,7 +125,7 @@ int oocd_swd_run(struct adiv5_dap *dap){ > // TODO: We are operating on global interface pointer, change it into > function parameter asap. > int oocd_swd_transport_init(struct command_context *ctx){ > LOG_DEBUG("entering function..."); > - int retval, *idcode; > + int *idcode, retval = ERROR_OK; Ideally try to always change as few bytes as possible in every patch. Some patches will be large anyway, but it is a good idea to not change "extra" things, such as the reordering of variable declarations here. It will work also to initialize retval if it remains first on the line, so this is preferable. Some diff algorithms are smart enough to visualize this change as only being new characters added after retval, so swapping the order adds noise. This change is probably perfectly fine, but in this case the patch itself is not enough for a reviewer to judge, since the only thing we see is your commit message saying "I changed it to return ERROR_OK where there was a warning for uninitialized variable" and we see this one more time in the patch, but we don't see the code that is affected by this change, so it's impossible to say if your fix is right or wrong. :\ The easiest way is to have more *why* and less *what* in the commit message. //Peter _______________________________________________ Openocd-development mailing list [email protected] https://lists.berlios.de/mailman/listinfo/openocd-development
