Re: [PATCH v3 1/3] cache: modify for "cat-file --literally -t"
- From
karthik nayak <karthik.188@gmail.com>
- Date
- Mar 9, 2015, 12:56 UTC
- Message-ID
- <54FD9898.8040801@gmail.com>
- In-Reply-To
- <CAPig+cT-AG2pnHJnkUcnHXzWu9jp2Q7ZBE7Re5jimFU7mOWLhQ@mail.gmail.com>
On 03/09/2015 03:55 AM, Eric Sunshine wrote:
Show 46 quoted lines
> On Thu, Mar 5, 2015 at 1:18 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> cache: modify for "cat-file --literally -t"
>
> It is desirable for the first line of the commit message to explain,
> as well as possible, the intent of the patch. The bulk of the commit
> message then elaborates. Unfortunately, this line says almost nothing.
> All patches modify, so writing "modify" here is not helpful and merely
> wastes precious horizontal real estate. A more informative summary
> might say something like:
>
> cache: add object_info::typename in support of 'cat-file --literally'
>
>> Add a "struct strbuf *typename" to object_info to hold the
>> typename when the literally option is used. Add a flag to
>> notify functions when literally is used.
>
> It's good to split up changes such that each patch comprises one
> logical step, however, this patch does not really do anything on its
> own, so having it stand-alone doesn't make much sense. It would make
> more sense to fold it into the patch which actually requires these
> changes.
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/cache.h b/cache.h
>> index 4d02efc..949ef4c 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -830,6 +830,7 @@ extern int is_ntfs_dotgit(const char *name);
>>
>> /* object replacement */
>> #define LOOKUP_REPLACE_OBJECT 1
>> +#define LOOKUP_LITERALLY 2
>> extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);
>> static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
>> {
>> @@ -1296,6 +1297,7 @@ struct object_info {
>> unsigned long *sizep;
>> unsigned long *disk_sizep;
>> unsigned char *delta_base_sha1;
>> + struct strbuf *typename;
>>
>> /* Response */
>> enum {
>> --
>> 2.3.1.167.g7f4ba4b.dirtyHey Eric! Thanks for the feedback, I guess I need to stick to different patches only for logical changes, not considering different files. Have been reading old commit messages to get a hang of it. Also found some blog posts explaining why we should use imperative sentences while writing Git commit messages. All makes sense now.
Regards -Karthik